On Wed, 13 Jan 2021 23:20:24 -0800 Cong Wang wrote: > On Tue, Jan 12, 2021 at 5:38 PM Jakub Kicinski <[email protected]> wrote: > > > > On Mon, 11 Jan 2021 18:55:48 -0800 Cong Wang wrote: > > > From: Cong Wang <[email protected]> > > > > > > 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: > > > [email protected] > > > Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options") > > > Fixes: 79b1011cb33d ("net: sched: allow flower to match erspan options") > > > Cc: Pieter Jansen van Vuuren <[email protected]> > > > Cc: Jamal Hadi Salim <[email protected]> > > > Cc: Xin Long <[email protected]> > > > Cc: Jiri Pirko <[email protected]> > > > Signed-off-by: Cong Wang <[email protected]> > > > > Thanks for keeping up with the syzbot bugs! > > > > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > > > index 1319986693fc..e265c443536e 100644 > > > --- a/net/sched/cls_flower.c > > > +++ b/net/sched/cls_flower.c > > > @@ -1272,6 +1272,8 @@ 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)) > > > + return -EINVAL; > > > > Can we just add another call to nla_validate_nested_deprecated() > > here instead of having to worry about each attr individually? > > No, we can not parse the nested attr here because different key types > have different attributes.
Not parse, just validate. Policy can be NULL, then nla_validate basically only checks nla_ok(). But my previous suggestion to call nla_validate_nested_deprecated() would not work, we'd need __nla_validate(NL_VALIDATE_TRAILING), so yeah, maybe that's more complex for a read to understand than just calling nla_ok()...
