From: Cong Wang <hidden> Date: 2021-01-12 02:56:51
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>
---
net/sched/cls_flower.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Xin Long <lucien.xin@gmail.com> Date: 2021-01-12 11:53:44
On Tue, Jan 12, 2021 at 10:56 AM Cong Wang [off-list ref] wrote:
quoted hunk
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>
---
net/sched/cls_flower.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Cong Wang <hidden> Date: 2021-01-12 17:44:20
On Tue, Jan 12, 2021 at 3:52 AM Xin Long [off-list ref] wrote:
On Tue, Jan 12, 2021 at 10:56 AM Cong Wang [off-list ref] wrote:
quoted
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>
---
net/sched/cls_flower.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
I think it's better to also add NL_SET_ERR_MSG(extack, xxxx);
for this error return, like all the other places in this function.
I think ext message is primarily for end users who usually can not
generate malformat netlink messages.
On the other hand, the nla_validate_nested_deprecated() right above
the quoted code does not set ext message either.
Thanks.
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>
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
}
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.
From: Xin Long <lucien.xin@gmail.com> Date: 2021-01-13 02:22:54
On Wed, Jan 13, 2021 at 1:43 AM Cong Wang [off-list ref] wrote:
On Tue, Jan 12, 2021 at 3:52 AM Xin Long [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 10:56 AM Cong Wang [off-list ref] wrote:
quoted
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>
---
net/sched/cls_flower.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
I think it's better to also add NL_SET_ERR_MSG(extack, xxxx);
for this error return, like all the other places in this function.
I think ext message is primarily for end users who usually can not
generate malformat netlink messages.
On the other hand, the nla_validate_nested_deprecated() right above
the quoted code does not set ext message either.
nla_validate_nested_deprecated(..., extack), it's already done inside
when returns error, no?
From: Cong Wang <hidden> Date: 2021-01-14 07:21:47
On Tue, Jan 12, 2021 at 5:38 PM Jakub Kicinski [off-list ref] wrote:
On Mon, 11 Jan 2021 18:55:48 -0800 Cong Wang wrote:
quoted
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>
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.
See below..
quoted
}
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.
Yeah, erroring out sounds better. Will change this in v2.
Thanks!
From: Cong Wang <hidden> Date: 2021-01-14 07:22:28
On Tue, Jan 12, 2021 at 6:22 PM Xin Long [off-list ref] wrote:
On Wed, Jan 13, 2021 at 1:43 AM Cong Wang [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 3:52 AM Xin Long [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 10:56 AM Cong Wang [off-list ref] wrote:
quoted
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>
---
net/sched/cls_flower.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
I think it's better to also add NL_SET_ERR_MSG(extack, xxxx);
for this error return, like all the other places in this function.
I think ext message is primarily for end users who usually can not
generate malformat netlink messages.
On the other hand, the nla_validate_nested_deprecated() right above
the quoted code does not set ext message either.
nla_validate_nested_deprecated(..., extack), it's already done inside
when returns error, no?
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-01-14 17:07:41
On Wed, 13 Jan 2021 23:20:24 -0800 Cong Wang wrote:
On Tue, Jan 12, 2021 at 5:38 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Mon, 11 Jan 2021 18:55:48 -0800 Cong Wang wrote:
quoted
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>
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()...