| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAM0EoM=uBjgBYdF7rPhk--L2n+_6858cNu8SON2_ZDRUSsOn6A@mail.gmail.com>
Date: Thu, 22 Jun 2023 14:56:56 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, eric.dumazet@...il.com,
syzbot <syzkaller@...glegroups.com>, Stephen Hemminger <stephen@...workplumber.org>,
Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH net] sch_netem: fix issues in netem_change() vs get_dist_table()
On Thu, Jun 22, 2023 at 2:15 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> In blamed commit, I missed that get_dist_table() was allocating
> memory using GFP_KERNEL, and acquiring qdisc lock to perform
> the swap of newly allocated table with current one.
>
> In this patch, get_dist_table() is allocating memory and
> copy user data before we acquire the qdisc lock.
>
> Then we perform swap operations while being protected by the lock.
>
> Note that after this patch netem_change() no longer can do partial changes.
> If an error is returned, qdisc conf is left unchanged.
>
> Fixes: 2174a08db80d ("sch_netem: acquire qdisc lock in netem_change()")
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Stephen Hemminger <stephen@...workplumber.org>
> Cc: Jamal Hadi Salim <jhs@...atatu.com>
> Cc: Simon Horman <simon.horman@...igine.com>
LGTM.
Acked-by: Jamal Hadi Salim <jhs@...atatu.com>
cheers,
jamal
> ---
> net/sched/sch_netem.c | 59 ++++++++++++++++++-------------------------
> 1 file changed, 25 insertions(+), 34 deletions(-)
>
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index e79be1b3e74da3c154f7ee23e16cc9e8da8f7106..b93ec2a3454ebceea559299f90533cbf1c0f7c26 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -773,12 +773,10 @@ static void dist_free(struct disttable *d)
> * signed 16 bit values.
> */
>
> -static int get_dist_table(struct Qdisc *sch, struct disttable **tbl,
> - const struct nlattr *attr)
> +static int get_dist_table(struct disttable **tbl, const struct nlattr *attr)
> {
> size_t n = nla_len(attr)/sizeof(__s16);
> const __s16 *data = nla_data(attr);
> - spinlock_t *root_lock;
> struct disttable *d;
> int i;
>
> @@ -793,13 +791,7 @@ static int get_dist_table(struct Qdisc *sch, struct disttable **tbl,
> for (i = 0; i < n; i++)
> d->table[i] = data[i];
>
> - root_lock = qdisc_root_sleeping_lock(sch);
> -
> - spin_lock_bh(root_lock);
> - swap(*tbl, d);
> - spin_unlock_bh(root_lock);
> -
> - dist_free(d);
> + *tbl = d;
> return 0;
> }
>
> @@ -956,6 +948,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
> {
> struct netem_sched_data *q = qdisc_priv(sch);
> struct nlattr *tb[TCA_NETEM_MAX + 1];
> + struct disttable *delay_dist = NULL;
> + struct disttable *slot_dist = NULL;
> struct tc_netem_qopt *qopt;
> struct clgstate old_clg;
> int old_loss_model = CLG_RANDOM;
> @@ -966,6 +960,18 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
> if (ret < 0)
> return ret;
>
> + if (tb[TCA_NETEM_DELAY_DIST]) {
> + ret = get_dist_table(&delay_dist, tb[TCA_NETEM_DELAY_DIST]);
> + if (ret)
> + goto table_free;
> + }
> +
> + if (tb[TCA_NETEM_SLOT_DIST]) {
> + ret = get_dist_table(&slot_dist, tb[TCA_NETEM_SLOT_DIST]);
> + if (ret)
> + goto table_free;
> + }
> +
> sch_tree_lock(sch);
> /* backup q->clg and q->loss_model */
> old_clg = q->clg;
> @@ -975,26 +981,17 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
> ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]);
> if (ret) {
> q->loss_model = old_loss_model;
> + q->clg = old_clg;
> goto unlock;
> }
> } else {
> q->loss_model = CLG_RANDOM;
> }
>
> - if (tb[TCA_NETEM_DELAY_DIST]) {
> - ret = get_dist_table(sch, &q->delay_dist,
> - tb[TCA_NETEM_DELAY_DIST]);
> - if (ret)
> - goto get_table_failure;
> - }
> -
> - if (tb[TCA_NETEM_SLOT_DIST]) {
> - ret = get_dist_table(sch, &q->slot_dist,
> - tb[TCA_NETEM_SLOT_DIST]);
> - if (ret)
> - goto get_table_failure;
> - }
> -
> + if (delay_dist)
> + swap(q->delay_dist, delay_dist);
> + if (slot_dist)
> + swap(q->slot_dist, slot_dist);
> sch->limit = qopt->limit;
>
> q->latency = PSCHED_TICKS2NS(qopt->latency);
> @@ -1044,17 +1041,11 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>
> unlock:
> sch_tree_unlock(sch);
> - return ret;
>
> -get_table_failure:
> - /* recover clg and loss_model, in case of
> - * q->clg and q->loss_model were modified
> - * in get_loss_clg()
> - */
> - q->clg = old_clg;
> - q->loss_model = old_loss_model;
> -
> - goto unlock;
> +table_free:
> + dist_free(delay_dist);
> + dist_free(slot_dist);
> + return ret;
> }
>
> static int netem_init(struct Qdisc *sch, struct nlattr *opt,
> --
> 2.41.0.178.g377b9f9a00-goog
>
Powered by blists - more mailing lists