Re: [Patch net] cls_flower: call nla_ok() before nla_next()
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-01-13 01:39:13
On Mon, 11 Jan 2021 18:55:48 -0800 Cong Wang wrote:
From: Cong Wang <redacted>
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: Pieter Jansen van Vuuren <redacted>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Xin Long <lucien.xin@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <redacted>Thanks for keeping up with the syzbot bugs!
quoted hunk ↗ jump to hunk
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? See below..
quoted hunk ↗ jump to hunk
} nla_for_each_attr(nla_opt_key, nla_enc_key,@@ -1308,7 +1310,7 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, return -EINVAL; } - if (msk_depth) + if (nla_ok(nla_opt_msk, msk_depth)) nla_opt_msk = nla_next(nla_opt_msk, &msk_depth);
Should we not error otherwise? if msk_depth && !nla_ok() then the message is clearly misformatted. If we don't error out we'll keep reusing the same mask over and over, while the intention of this code was to have mask per key AFAICT.
break; case TCA_FLOWER_KEY_ENC_OPTS_VXLAN: