| 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: <20230822074850.GS2711035@kernel.org>
Date: Tue, 22 Aug 2023 09:48:50 +0200
From: Simon Horman <horms@...nel.org>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
edumazet@...gle.com, pabeni@...hat.com, jiri@...nulli.us,
xiyou.wangcong@...il.com, syzkaller-bugs@...glegroups.com,
linux-kernel@...r.kernel.org, shaozhengchao@...wei.com,
syzbot+a3618a167af2021433cd@...kaller.appspotmail.com,
Vladimir Oltean <vladimir.oltean@....com>,
Victor Nogueira <victor@...atatu.com>,
Pedro Tammela <pctammela@...atatu.com>
Subject: Re: [PATCH net v2 1/1] net/sched: fix a qdisc modification with
ambiguous command request
On Mon, Aug 21, 2023 at 03:13:05PM -0400, Jamal Hadi Salim wrote:
> When replacing an existing root qdisc, with one that is of the same kind, the
> request boils down to essentially a parameterization change i.e not one that
> requires allocation and grafting of a new qdisc. syzbot was able to create a
> scenario which resulted in a taprio qdisc replacing an existing taprio qdisc
> with a combination of NLM_F_CREATE, NLM_F_REPLACE and NLM_F_EXCL leading to
> create and graft scenario.
> The fix ensures that only when the qdisc kinds are different that we should
> allow a create and graft, otherwise it goes into the "change" codepath.
>
> While at it, fix the code and comments to improve readability.
>
> While syzbot was able to create the issue, it did not zone on the root cause.
> Analysis from Vladimir Oltean <vladimir.oltean@....com> helped narrow it down.
>
> v1->V2 changes:
> - remove "inline" function definition (Vladmir)
> - remove extrenous braces in branches (Vladmir)
> - change inline function names (Pedro)
> - Run tdc tests (Victor)
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+a3618a167af2021433cd@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/20230816225759.g25x76kmgzya2gei@skbuf/T/
> Tested-by: Vladimir Oltean <vladimir.oltean@....com>
> Tested-by: Victor Nogueira <victor@...atatu.com>
> Reviewed-by: Pedro Tammela <pctammela@...atatu.com>
> Reviewed-by: Victor Nogueira <victor@...atatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
> ---
> net/sched/sch_api.c | 54 ++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index aa6b1fe65151..4c51b8bef1b8 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1547,10 +1547,28 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> return 0;
> }
>
> +static bool req_create_or_replace(struct nlmsghdr *n)
> +{
> + return (n->nlmsg_flags & NLM_F_CREATE &&
> + n->nlmsg_flags & NLM_F_REPLACE);
> +}
> +
> +static bool req_create_exclusive(struct nlmsghdr *n)
> +{
> + return (n->nlmsg_flags & NLM_F_CREATE &&
> + n->nlmsg_flags & NLM_F_EXCL);
> +}
> +
> +static bool req_change(struct nlmsghdr *n)
> +{
> + return (!(n->nlmsg_flags & NLM_F_CREATE) &&
> + !(n->nlmsg_flags & NLM_F_REPLACE) &&
> + !(n->nlmsg_flags & NLM_F_EXCL));
> +}
> +
> /*
> * Create/change qdisc.
> */
> -
> static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> struct netlink_ext_ack *extack)
> {
> @@ -1644,27 +1662,36 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> *
> * We know, that some child q is already
> * attached to this parent and have choice:
> - * either to change it or to create/graft new one.
> + * 1) change it or 2) create/graft new one.
> + * If the requested qdisc kind is different
> + * than the existing one, then we choose graft.
> + * If they are the same then this is "change"
> + * operation - just let it fallthrough..
> *
> * 1. We are allowed to create/graft only
> - * if CREATE and REPLACE flags are set.
> + * if the request is explicitly stating
> + * "please create if it doesn't exist".
> *
> - * 2. If EXCL is set, requestor wanted to say,
> - * that qdisc tcm_handle is not expected
> + * 2. If the request is to exclusive create
> + * then the qdisc tcm_handle is not expected
> * to exist, so that we choose create/graft too.
> *
> * 3. The last case is when no flags are set.
> + * This will happen when for example tc
> + * utility issues a "change" command.
> * Alas, it is sort of hole in API, we
> * cannot decide what to do unambiguously.
> - * For now we select create/graft, if
> - * user gave KIND, which does not match existing.
> + * For now we select create/graft.
> */
> - if ((n->nlmsg_flags & NLM_F_CREATE) &&
> - (n->nlmsg_flags & NLM_F_REPLACE) &&
> - ((n->nlmsg_flags & NLM_F_EXCL) ||
> - (tca[TCA_KIND] &&
> - nla_strcmp(tca[TCA_KIND], q->ops->id))))
> - goto create_n_graft;
> + if (tca[TCA_KIND] &&
> + nla_strcmp(tca[TCA_KIND], q->ops->id)) {
> + if (req_create_or_replace(n) ||
> + req_create_exclusive(n))
> + goto create_n_graft;
> + else
> + if (req_change(n))
Hi Jamal,
a minor nit from my side, which also came up in v1:
this could be else if, avoiding one level of indentation.
> + goto create_n_graft2;
> + }
> }
> }
> } else {
> @@ -1698,6 +1725,7 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> NL_SET_ERR_MSG(extack, "Qdisc not found. To create specify NLM_F_CREATE flag");
> return -ENOENT;
> }
> +create_n_graft2:
> if (clid == TC_H_INGRESS) {
> if (dev_ingress_queue(dev)) {
> q = qdisc_create(dev, dev_ingress_queue(dev),
> --
> 2.34.1
>
>
Powered by blists - more mailing lists