| Message ID | 20201028170731.1383332-1-aleksandrnogikh@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] netem: fix zero division in tabledist | expand |
On Wed, 28 Oct 2020 17:07:31 +0000 Aleksandr Nogikh <aleksandrnogikh@gmail.com> wrote: > From: Aleksandr Nogikh <nogikh@google.com> > > Currently it is possible to craft a special netlink RTM_NEWQDISC > command that can result in jitter being equal to 0x80000000. It is > enough to set the 32 bit jitter to 0x02000000 (it will later be > multiplied by 2^6) or just set the 64 bit jitter via > TCA_NETEM_JITTER64. This causes an overflow during the generation of > uniformly distributed numbers in tabledist(), which in turn leads to > division by zero (sigma != 0, but sigma * 2 is 0). > > The related fragment of code needs 32-bit division - see commit > 9b0ed89 ("netem: remove unnecessary 64 bit modulus"), so switching to > 64 bit is not an option. > > Fix the issue by keeping the value of jitter within the range that can > be adequately handled by tabledist() - [0;INT_MAX]. As negative std > deviation makes no sense, take the absolute value of the passed value > and cap it at INT_MAX. Inside tabledist(), switch to unsigned 32 bit > arithmetic in order to prevent overflows. > > Signed-off-by: Aleksandr Nogikh <nogikh@google.com> > Reported-by: syzbot+ec762a6342ad0d3c0d8f@syzkaller.appspotmail.com Acked-by: Stephen Hemminger <stephen@networkplumber.org>
On Wed, 28 Oct 2020 11:19:59 -0700 Stephen Hemminger wrote: > On Wed, 28 Oct 2020 17:07:31 +0000 > Aleksandr Nogikh <aleksandrnogikh@gmail.com> wrote: > > > From: Aleksandr Nogikh <nogikh@google.com> > > > > Currently it is possible to craft a special netlink RTM_NEWQDISC > > command that can result in jitter being equal to 0x80000000. It is > > enough to set the 32 bit jitter to 0x02000000 (it will later be > > multiplied by 2^6) or just set the 64 bit jitter via > > TCA_NETEM_JITTER64. This causes an overflow during the generation of > > uniformly distributed numbers in tabledist(), which in turn leads to > > division by zero (sigma != 0, but sigma * 2 is 0). > > > > The related fragment of code needs 32-bit division - see commit > > 9b0ed89 ("netem: remove unnecessary 64 bit modulus"), so switching to > > 64 bit is not an option. > > > > Fix the issue by keeping the value of jitter within the range that can > > be adequately handled by tabledist() - [0;INT_MAX]. As negative std > > deviation makes no sense, take the absolute value of the passed value > > and cap it at INT_MAX. Inside tabledist(), switch to unsigned 32 bit > > arithmetic in order to prevent overflows. > > > > Signed-off-by: Aleksandr Nogikh <nogikh@google.com> > > Reported-by: syzbot+ec762a6342ad0d3c0d8f@syzkaller.appspotmail.com > > Acked-by: Stephen Hemminger <stephen@networkplumber.org> Applied, thanks!
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c index 84f82771cdf5..0c345e43a09a 100644 --- a/net/sched/sch_netem.c +++ b/net/sched/sch_netem.c @@ -330,7 +330,7 @@ static s64 tabledist(s64 mu, s32 sigma, /* default uniform distribution */ if (dist == NULL) - return ((rnd % (2 * sigma)) + mu) - sigma; + return ((rnd % (2 * (u32)sigma)) + mu) - sigma; t = dist->table[rnd % dist->size]; x = (sigma % NETEM_DIST_SCALE) * t; @@ -812,6 +812,10 @@ static void get_slot(struct netem_sched_data *q, const struct nlattr *attr) q->slot_config.max_packets = INT_MAX; if (q->slot_config.max_bytes == 0) q->slot_config.max_bytes = INT_MAX; + + /* capping dist_jitter to the range acceptable by tabledist() */ + q->slot_config.dist_jitter = min_t(__s64, INT_MAX, abs(q->slot_config.dist_jitter)); + q->slot.packets_left = q->slot_config.max_packets; q->slot.bytes_left = q->slot_config.max_bytes; if (q->slot_config.min_delay | q->slot_config.max_delay | @@ -1037,6 +1041,9 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt, if (tb[TCA_NETEM_SLOT]) get_slot(q, tb[TCA_NETEM_SLOT]); + /* capping jitter to the range acceptable by tabledist() */ + q->jitter = min_t(s64, abs(q->jitter), INT_MAX); + return ret; get_table_failure: