| 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: <20230819143015.yjegbsu32ddsp3fb@skbuf>
Date: Sat, 19 Aug 2023 17:30:15 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
pabeni@...hat.com, jiri@...nulli.us, xiyou.wangcong@...il.com,
netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com,
linux-kernel@...r.kernel.org, shaozhengchao@...wei.com,
syzbot+a3618a167af2021433cd@...kaller.appspotmail.com
Subject: Re: [PATCH net 1/1] net/sched: fix a qdisc modification with
ambigous command request
Hi Jamal,
There's a typo in the title: ambigous -> ambiguous
On Fri, Aug 18, 2023 at 02:14:32PM -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 paremeterization 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.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+a3618a167af2021433cd@...kaller.appspotmail.com
> Closes:https://lore.kernel.org/netdev/20230816225759.g25x76kmgzya2gei@skbuf/T/
Space between Closes: tag and link.
> Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
Tested-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> net/sched/sch_api.c | 55 ++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index aa6b1fe65151..dd3db8608275 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 inline bool cmd_create_or_replace(struct nlmsghdr *n)
We tend to use "static inline" functions only when defining them in headers.
In C files, we drop "inline" and let the compiler decide whether to inline or not.
> +{
> + return (n->nlmsg_flags & NLM_F_CREATE &&
> + n->nlmsg_flags & NLM_F_REPLACE);
> +}
> +
> +static inline bool cmd_create_exclusive(struct nlmsghdr *n)
> +{
> + return (n->nlmsg_flags & NLM_F_CREATE &&
> + n->nlmsg_flags & NLM_F_EXCL);
> +}
> +
> +static inline bool cmd_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,37 @@ 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 (cmd_create_or_replace(n) ||
> + cmd_create_exclusive(n)) {
> + goto create_n_graft;
> + } else {
> + if (cmd_change(n))
else if cmd_change()
thus the code block under the qdisc kind comparison can become:
if (cmd_create_or_replace(n) ||
cmd_create_exclusive(n))
goto create_n_graft;
else if (cmd_change(n))
goto create_n_graft2;
(no extra brackets are needed)
> + goto create_n_graft2;
> + }
> + }
> }
> }
> } else {
> @@ -1698,6 +1726,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