There is a pattern of bugs that end up creating UAFs or null ptr derefs. The majority of these bugs follow the formula below: a) create a nonsense hierarchy of qdiscs which has no practical value, b) start sending packets Optional c) netlink cmds to change hierarchy some more; It's more fun if you can get packets stuck - the formula in this case includes non work-conserving qdiscs somewhere in the hierarchy Optional d dependent on c) send more packets e) profit Current init/change qdisc APIs are localised to validate only within the constraint of a single qdisc. So catching #a or #c is a challenge. Our policy, when said bugs are presented, is to "make it work" by modifying generally used data structures and code, but these come at the expense of adding special checks for corner cases which are nonsensical to begin with. The goal of this patchset is to create an equivalent to PCI quirks, which will catch nonsensical hierarchies in #a and #c and reject such a config. With that in mind, we are proposing the addition of a new qdisc op (quirk_chk). We introduce, as a first example, the quirk_chk op to netem. Its purpose here is to validate whether the user is attempting to add nested netem duplicates in the same qdisc tree branch which will be forbidden. Here is an example that should now work: DEV="eth0" NUM_QUEUES=4 DUPLICATE_PERCENT="5%" tc qdisc del dev $DEV root > /dev/null 2>&1 tc qdisc add dev $DEV root handle 1: mq for i in $(seq 1 $NUM_QUEUES); do HANDLE_ID=$((i * 10)) PARENT_ID="1:$i" tc qdisc add dev $DEV parent $PARENT_ID handle \ ${HANDLE_ID}: netem duplicate $DUPLICATE_PERCENT done Suggested-by: Jamal Hadi Salim Signed-off-by: Victor Nogueira --- v2 -> v3: - Restrict netem duplicate nested on the same qdisc tree branch instead of the whole tree - Use qdisc_lookup to iterate through qdisc ancestors - Check whether opt is NULL directly in netem_quirk_chk Link to v2: https://lore.kernel.org/netdev/20251125224604.872351-1-victor@mojatatu.com/ v1 -> v2: - Simplify process of getting root qdisc in netem_quirk_chk - Use parent's major directly instead of looking up parent qdisc in netem_quirk_chk - Call parse_attrs in netem_quirk_chk to avoid issue caught by syzbot Link to v1: https://lore.kernel.org/netdev/20251124223749.503979-1-victor@mojatatu.com/ --- include/net/sch_generic.h | 3 +++ net/sched/sch_api.c | 13 ++++++++++ net/sched/sch_netem.c | 54 ++++++++++++++++++++++----------------- 3 files changed, 46 insertions(+), 24 deletions(-) diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 94966692ccdf..60450372c5d5 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -313,6 +313,9 @@ struct Qdisc_ops { u32 block_index); void (*egress_block_set)(struct Qdisc *sch, u32 block_index); + int (*quirk_chk)(struct Qdisc *sch, + struct nlattr *arg, + struct netlink_ext_ack *extack); u32 (*ingress_block_get)(struct Qdisc *sch); u32 (*egress_block_get)(struct Qdisc *sch); diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index f56b18c8aebf..73190d8ca792 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -316,6 +316,7 @@ struct Qdisc *qdisc_lookup(struct net_device *dev, u32 handle) out: return q; } +EXPORT_SYMBOL_GPL(qdisc_lookup); struct Qdisc *qdisc_lookup_rcu(struct net_device *dev, u32 handle) { @@ -1315,6 +1316,12 @@ static struct Qdisc *qdisc_create(struct net_device *dev, rcu_assign_pointer(sch->stab, stab); } + if (ops->quirk_chk) { + err = ops->quirk_chk(sch, tca[TCA_OPTIONS], extack); + if (err != 0) + goto err_out3; + } + if (ops->init) { err = ops->init(sch, tca[TCA_OPTIONS], extack); if (err != 0) @@ -1378,6 +1385,12 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca, NL_SET_ERR_MSG(extack, "Change of blocks is not supported"); return -EOPNOTSUPP; } + if (sch->ops->quirk_chk) { + err = sch->ops->quirk_chk(sch, tca[TCA_OPTIONS], + extack); + if (err != 0) + return err; + } err = sch->ops->change(sch, tca[TCA_OPTIONS], extack); if (err) return err; diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c index eafc316ae319..6ddc9250ca88 100644 --- a/net/sched/sch_netem.c +++ b/net/sched/sch_netem.c @@ -975,36 +975,46 @@ static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla, static const struct Qdisc_class_ops netem_class_ops; -static int check_netem_in_tree(struct Qdisc *sch, bool duplicates, - struct netlink_ext_ack *extack) +static int netem_quirk_chk(struct Qdisc *sch, struct nlattr *opt, + struct netlink_ext_ack *extack) { - struct Qdisc *root, *q; - unsigned int i; + struct tc_netem_qopt *qopt; + bool duplicates; + struct Qdisc *q; + u32 parentid; + + if (!opt) + return -EINVAL; - root = qdisc_root_sleeping(sch); + qopt = nla_data(opt); + duplicates = qopt->duplicate; - if (sch != root && root->ops->cl_ops == &netem_class_ops) { - if (duplicates || - ((struct netem_sched_data *)qdisc_priv(root))->duplicate) - goto err; - } + if (duplicates) { + q = sch; + while ((parentid = q->parent)) { + if (parentid == TC_H_ROOT) + break; - if (!qdisc_dev(root)) - return 0; + if (q->flags & TCQ_F_NOPARENT) + break; - hash_for_each(qdisc_dev(root)->qdisc_hash, i, q, hash) { - if (sch != q && q->ops->cl_ops == &netem_class_ops) { - if (duplicates || - ((struct netem_sched_data *)qdisc_priv(q))->duplicate) - goto err; + q = qdisc_lookup(qdisc_dev(q), TC_H_MAJ(parentid)); + if (!q) + break; + + if (q->ops->cl_ops == &netem_class_ops) { + struct netem_sched_data *priv = qdisc_priv(q); + + if (priv->duplicate) + goto err; + } } } return 0; err: - NL_SET_ERR_MSG(extack, - "netem: cannot mix duplicating netems with other netems in tree"); + NL_SET_ERR_MSG_MOD(extack, "cannot have nested netem duplicates"); return -EINVAL; } @@ -1066,11 +1076,6 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt, q->gap = qopt->gap; q->counter = 0; q->loss = qopt->loss; - - ret = check_netem_in_tree(sch, qopt->duplicate, extack); - if (ret) - goto unlock; - q->duplicate = qopt->duplicate; /* for compatibility with earlier versions. @@ -1352,6 +1357,7 @@ static struct Qdisc_ops netem_qdisc_ops __read_mostly = { .destroy = netem_destroy, .change = netem_change, .dump = netem_dump, + .quirk_chk = netem_quirk_chk, .owner = THIS_MODULE, }; MODULE_ALIAS_NET_SCH("netem"); -- 2.51.0