* [Patch net v3] cls_flower: call nla_ok() before nla_next()
@ 2021-01-14 21:07 Cong Wang
2021-01-14 21:36 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2021-01-14 21:07 UTC (permalink / raw
To: netdev
Cc: Cong Wang, syzbot+2624e3778b18fc497c92, Jamal Hadi Salim,
Xin Long, Jiri Pirko, Jakub Kicinski
From: Cong Wang <cong.wang@bytedance.com>
fl_set_enc_opt() simply checks if there are still bytes left to parse,
but this is not sufficent as syzbot seems to be able to generate
malformatted netlink messages. nla_ok() is more strict so should be
used to validate the next nlattr here.
And nla_validate_nested_deprecated() has less strict check too, it is
probably too late to switch to the strict version, but we can just
call nla_ok() too after it.
Reported-and-tested-by: syzbot+2624e3778b18fc497c92@syzkaller.appspotmail.com
Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options")
Fixes: 79b1011cb33d ("net: sched: allow flower to match erspan options")
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Xin Long <lucien.xin@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
net/sched/cls_flower.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 1319986693fc..280eab0c833f 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1272,6 +1272,10 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key,
nla_opt_msk = nla_data(tb[TCA_FLOWER_KEY_ENC_OPTS_MASK]);
msk_depth = nla_len(tb[TCA_FLOWER_KEY_ENC_OPTS_MASK]);
+ if (!nla_ok(nla_opt_msk, msk_depth)) {
+ NL_SET_ERR_MSG(extack, "Invalid attribute for masks");
+ return -EINVAL;
+ }
}
nla_for_each_attr(nla_opt_key, nla_enc_key,
@@ -1307,9 +1311,6 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key,
NL_SET_ERR_MSG(extack, "Key and mask miss aligned");
return -EINVAL;
}
-
- if (msk_depth)
- nla_opt_msk = nla_next(nla_opt_msk, &msk_depth);
break;
case TCA_FLOWER_KEY_ENC_OPTS_VXLAN:
if (key->enc_opts.dst_opt_type) {
@@ -1340,9 +1341,6 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key,
NL_SET_ERR_MSG(extack, "Key and mask miss aligned");
return -EINVAL;
}
-
- if (msk_depth)
- nla_opt_msk = nla_next(nla_opt_msk, &msk_depth);
break;
case TCA_FLOWER_KEY_ENC_OPTS_ERSPAN:
if (key->enc_opts.dst_opt_type) {
@@ -1373,14 +1371,20 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key,
NL_SET_ERR_MSG(extack, "Key and mask miss aligned");
return -EINVAL;
}
-
- if (msk_depth)
- nla_opt_msk = nla_next(nla_opt_msk, &msk_depth);
break;
default:
NL_SET_ERR_MSG(extack, "Unknown tunnel option type");
return -EINVAL;
}
+
+ if (!nla_opt_msk)
+ continue;
+
+ if (!nla_ok(nla_opt_msk, msk_depth)) {
+ NL_SET_ERR_MSG(extack, "A mask attribute is invalid");
+ return -EINVAL;
+ }
+ nla_opt_msk = nla_next(nla_opt_msk, &msk_depth);
}
return 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch net v3] cls_flower: call nla_ok() before nla_next()
2021-01-14 21:07 [Patch net v3] cls_flower: call nla_ok() before nla_next() Cong Wang
@ 2021-01-14 21:36 ` Jakub Kicinski
2021-01-14 21:57 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2021-01-14 21:36 UTC (permalink / raw
To: Cong Wang
Cc: netdev, Cong Wang, syzbot+2624e3778b18fc497c92, Jamal Hadi Salim,
Xin Long, Jiri Pirko
On Thu, 14 Jan 2021 13:07:49 -0800 Cong Wang wrote:
> - if (msk_depth)
> - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth);
> break;
> default:
> NL_SET_ERR_MSG(extack, "Unknown tunnel option type");
> return -EINVAL;
> }
> +
> + if (!nla_opt_msk)
> + continue;
Why the switch from !msk_depth to !nla_opt_msk?
Seems like previously providing masks for only subset of options
would have worked.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch net v3] cls_flower: call nla_ok() before nla_next()
2021-01-14 21:36 ` Jakub Kicinski
@ 2021-01-14 21:57 ` Cong Wang
2021-01-14 22:30 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2021-01-14 21:57 UTC (permalink / raw
To: Jakub Kicinski
Cc: Linux Kernel Network Developers, Cong Wang, syzbot,
Jamal Hadi Salim, Xin Long, Jiri Pirko
On Thu, Jan 14, 2021 at 1:36 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 14 Jan 2021 13:07:49 -0800 Cong Wang wrote:
> > - if (msk_depth)
> > - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth);
> > break;
> > default:
> > NL_SET_ERR_MSG(extack, "Unknown tunnel option type");
> > return -EINVAL;
> > }
> > +
> > + if (!nla_opt_msk)
> > + continue;
>
> Why the switch from !msk_depth to !nla_opt_msk?
It is the same, when nla_opt_msk is NULL, msk_depth is 0.
Checking nla_opt_msk is NULL is more readable to express that
mask is not provided.
>
> Seems like previously providing masks for only subset of options
> would have worked.
I don't think so, every type has this check:
if (key->enc_opts.len != mask->enc_opts.len) {
NL_SET_ERR_MSG(extack, "Key and mask
miss aligned");
return -EINVAL;
}
which guarantees the numbers are aligned.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch net v3] cls_flower: call nla_ok() before nla_next()
2021-01-14 21:57 ` Cong Wang
@ 2021-01-14 22:30 ` Jakub Kicinski
2021-01-14 22:50 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2021-01-14 22:30 UTC (permalink / raw
To: Cong Wang
Cc: Linux Kernel Network Developers, Cong Wang, syzbot,
Jamal Hadi Salim, Xin Long, Jiri Pirko
On Thu, 14 Jan 2021 13:57:13 -0800 Cong Wang wrote:
> On Thu, Jan 14, 2021 at 1:36 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu, 14 Jan 2021 13:07:49 -0800 Cong Wang wrote:
> > > - if (msk_depth)
> > > - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth);
> > > break;
> > > default:
> > > NL_SET_ERR_MSG(extack, "Unknown tunnel option type");
> > > return -EINVAL;
> > > }
> > > +
> > > + if (!nla_opt_msk)
> > > + continue;
> >
> > Why the switch from !msk_depth to !nla_opt_msk?
>
> It is the same, when nla_opt_msk is NULL, msk_depth is 0.
> Checking nla_opt_msk is NULL is more readable to express that
> mask is not provided.
>
> >
> > Seems like previously providing masks for only subset of options
> > would have worked.
>
> I don't think so, every type has this check:
>
> if (key->enc_opts.len != mask->enc_opts.len) {
> NL_SET_ERR_MSG(extack, "Key and mask
> miss aligned");
> return -EINVAL;
> }
>
> which guarantees the numbers are aligned.
>
> Thanks.
static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key,
int depth, int option_len,
struct netlink_ext_ack *extack)
{
struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_VXLAN_MAX + 1];
struct vxlan_metadata *md;
int err;
md = (struct vxlan_metadata *)&key->enc_opts.data[key->enc_opts.len];
memset(md, 0xff, sizeof(*md));
if (!depth)
return sizeof(*md);
^^^^^^^^^^^^^^^^^^^
The mask is filled with all 1s if attribute is not provided.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch net v3] cls_flower: call nla_ok() before nla_next()
2021-01-14 22:30 ` Jakub Kicinski
@ 2021-01-14 22:50 ` Cong Wang
2021-01-14 22:56 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2021-01-14 22:50 UTC (permalink / raw
To: Jakub Kicinski
Cc: Linux Kernel Network Developers, Cong Wang, syzbot,
Jamal Hadi Salim, Xin Long, Jiri Pirko
On Thu, Jan 14, 2021 at 2:30 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 14 Jan 2021 13:57:13 -0800 Cong Wang wrote:
> > On Thu, Jan 14, 2021 at 1:36 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Thu, 14 Jan 2021 13:07:49 -0800 Cong Wang wrote:
> > > > - if (msk_depth)
> > > > - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth);
> > > > break;
> > > > default:
> > > > NL_SET_ERR_MSG(extack, "Unknown tunnel option type");
> > > > return -EINVAL;
> > > > }
> > > > +
> > > > + if (!nla_opt_msk)
> > > > + continue;
> > >
> > > Why the switch from !msk_depth to !nla_opt_msk?
> >
> > It is the same, when nla_opt_msk is NULL, msk_depth is 0.
> > Checking nla_opt_msk is NULL is more readable to express that
> > mask is not provided.
> >
> > >
> > > Seems like previously providing masks for only subset of options
> > > would have worked.
> >
> > I don't think so, every type has this check:
> >
> > if (key->enc_opts.len != mask->enc_opts.len) {
> > NL_SET_ERR_MSG(extack, "Key and mask
> > miss aligned");
> > return -EINVAL;
> > }
> >
> > which guarantees the numbers are aligned.
> >
> > Thanks.
>
> static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key,
> int depth, int option_len,
> struct netlink_ext_ack *extack)
> {
> struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_VXLAN_MAX + 1];
> struct vxlan_metadata *md;
> int err;
>
> md = (struct vxlan_metadata *)&key->enc_opts.data[key->enc_opts.len];
> memset(md, 0xff, sizeof(*md));
>
> if (!depth)
> return sizeof(*md);
> ^^^^^^^^^^^^^^^^^^^
>
> The mask is filled with all 1s if attribute is not provided.
Hmm, then what is the length comparison check for?
fl_set_vxlan_opt() either turns negative or sizeof(*md), and negitve
is already checked when it returns, so when we hit the length comparison
it is always equal. So it must be redundant.
(Note, I am only talking about the vxlan case you pick here, because the
geneve case is different, as it returns different sizes.)
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch net v3] cls_flower: call nla_ok() before nla_next()
2021-01-14 22:50 ` Cong Wang
@ 2021-01-14 22:56 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2021-01-14 22:56 UTC (permalink / raw
To: Cong Wang
Cc: Linux Kernel Network Developers, Cong Wang, syzbot,
Jamal Hadi Salim, Xin Long, Jiri Pirko
On Thu, 14 Jan 2021 14:50:04 -0800 Cong Wang wrote:
> > static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key,
> > int depth, int option_len,
> > struct netlink_ext_ack *extack)
> > {
> > struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_VXLAN_MAX + 1];
> > struct vxlan_metadata *md;
> > int err;
> >
> > md = (struct vxlan_metadata *)&key->enc_opts.data[key->enc_opts.len];
> > memset(md, 0xff, sizeof(*md));
> >
> > if (!depth)
> > return sizeof(*md);
> > ^^^^^^^^^^^^^^^^^^^
> >
> > The mask is filled with all 1s if attribute is not provided.
>
> Hmm, then what is the length comparison check for?
>
> fl_set_vxlan_opt() either turns negative or sizeof(*md), and negitve
> is already checked when it returns, so when we hit the length comparison
> it is always equal. So it must be redundant.
>
> (Note, I am only talking about the vxlan case you pick here, because the
> geneve case is different, as it returns different sizes.)
Good question 🤷🏻♂️🤷🏻♂️ I'd guess GENEVE came first and and then
people copy / pasted the code structure other tunnel types?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-14 22:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-14 21:07 [Patch net v3] cls_flower: call nla_ok() before nla_next() Cong Wang
2021-01-14 21:36 ` Jakub Kicinski
2021-01-14 21:57 ` Cong Wang
2021-01-14 22:30 ` Jakub Kicinski
2021-01-14 22:50 ` Cong Wang
2021-01-14 22:56 ` Jakub Kicinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.