Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [Patch net] cls_flower: call nla_ok() before nla_next()
@ 2021-01-12  2:55 Cong Wang
  2021-01-12 11:52 ` Xin Long
  2021-01-13  1:38 ` Jakub Kicinski
  0 siblings, 2 replies; 8+ messages in thread
From: Cong Wang @ 2021-01-12  2:55 UTC (permalink / raw)
  To: netdev
  Cc: Cong Wang, syzbot+2624e3778b18fc497c92, Pieter Jansen van Vuuren,
	Jamal Hadi Salim, Xin Long, Jiri Pirko

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: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
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 <cong.wang@bytedance.com>
---
 net/sched/cls_flower.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

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;
 	}
 
 	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);
 			break;
 		case TCA_FLOWER_KEY_ENC_OPTS_VXLAN:
@@ -1341,7 +1343,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);
 			break;
 		case TCA_FLOWER_KEY_ENC_OPTS_ERSPAN:
@@ -1374,7 +1376,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);
 			break;
 		default:
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch net] cls_flower: call nla_ok() before nla_next()
  2021-01-12  2:55 [Patch net] cls_flower: call nla_ok() before nla_next() Cong Wang
@ 2021-01-12 11:52 ` Xin Long
  2021-01-12 17:43   ` Cong Wang
  2021-01-13  1:38 ` Jakub Kicinski
  1 sibling, 1 reply; 8+ messages in thread
From: Xin Long @ 2021-01-12 11:52 UTC (permalink / raw)
  To: Cong Wang
  Cc: network dev, Cong Wang, syzbot+2624e3778b18fc497c92,
	Pieter Jansen van Vuuren, Jamal Hadi Salim, Jiri Pirko

On Tue, Jan 12, 2021 at 10:56 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> 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: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> 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 <cong.wang@bytedance.com>
> ---
>  net/sched/cls_flower.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> 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;
>         }
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.

>
>         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);
>                         break;
>                 case TCA_FLOWER_KEY_ENC_OPTS_VXLAN:
> @@ -1341,7 +1343,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);
>                         break;
>                 case TCA_FLOWER_KEY_ENC_OPTS_ERSPAN:
> @@ -1374,7 +1376,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);
>                         break;
>                 default:
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch net] cls_flower: call nla_ok() before nla_next()
  2021-01-12 11:52 ` Xin Long
@ 2021-01-12 17:43   ` Cong Wang
  2021-01-13  2:22     ` Xin Long
  0 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2021-01-12 17:43 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, Cong Wang, syzbot, Jamal Hadi Salim, Jiri Pirko

On Tue, Jan 12, 2021 at 3:52 AM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Tue, Jan 12, 2021 at 10:56 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > 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: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > 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 <cong.wang@bytedance.com>
> > ---
> >  net/sched/cls_flower.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > 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;
> >         }
> 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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch net] cls_flower: call nla_ok() before nla_next()
  2021-01-12  2:55 [Patch net] cls_flower: call nla_ok() before nla_next() Cong Wang
  2021-01-12 11:52 ` Xin Long
@ 2021-01-13  1:38 ` Jakub Kicinski
  2021-01-14  7:20   ` Cong Wang
  1 sibling, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2021-01-13  1:38 UTC (permalink / raw)
  To: Cong Wang
  Cc: netdev, Cong Wang, syzbot+2624e3778b18fc497c92,
	Pieter Jansen van Vuuren, Jamal Hadi Salim, Xin Long, Jiri Pirko

On Mon, 11 Jan 2021 18:55:48 -0800 Cong Wang wrote:
> 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: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> 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 <cong.wang@bytedance.com>

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?
See below..

>  	}
>  
>  	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:

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch net] cls_flower: call nla_ok() before nla_next()
  2021-01-12 17:43   ` Cong Wang
@ 2021-01-13  2:22     ` Xin Long
  2021-01-14  7:21       ` Cong Wang
  0 siblings, 1 reply; 8+ messages in thread
From: Xin Long @ 2021-01-13  2:22 UTC (permalink / raw)
  To: Cong Wang; +Cc: network dev, Cong Wang, syzbot, Jamal Hadi Salim, Jiri Pirko

On Wed, Jan 13, 2021 at 1:43 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Tue, Jan 12, 2021 at 3:52 AM Xin Long <lucien.xin@gmail.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 10:56 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > 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: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > > 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 <cong.wang@bytedance.com>
> > > ---
> > >  net/sched/cls_flower.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > 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;
> > >         }
> > 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?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch net] cls_flower: call nla_ok() before nla_next()
  2021-01-13  1:38 ` Jakub Kicinski
@ 2021-01-14  7:20   ` Cong Wang
  2021-01-14 17:06     ` Jakub Kicinski
  0 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2021-01-14  7:20 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Linux Kernel Network Developers, Cong Wang, syzbot,
	Jamal Hadi Salim, Xin Long, Jiri Pirko

On Tue, Jan 12, 2021 at 5:38 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 11 Jan 2021 18:55:48 -0800 Cong Wang wrote:
> > 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: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > 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 <cong.wang@bytedance.com>
>
> 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.

> See below..
>
> >       }
> >
> >       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!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch net] cls_flower: call nla_ok() before nla_next()
  2021-01-13  2:22     ` Xin Long
@ 2021-01-14  7:21       ` Cong Wang
  0 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2021-01-14  7:21 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, Cong Wang, syzbot, Jamal Hadi Salim, Jiri Pirko

On Tue, Jan 12, 2021 at 6:22 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Wed, Jan 13, 2021 at 1:43 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 3:52 AM Xin Long <lucien.xin@gmail.com> wrote:
> > >
> > > On Tue, Jan 12, 2021 at 10:56 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > >
> > > > 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: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > > > 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 <cong.wang@bytedance.com>
> > > > ---
> > > >  net/sched/cls_flower.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > 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;
> > > >         }
> > > 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?

Yeah, seems fair.

Thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch net] cls_flower: call nla_ok() before nla_next()
  2021-01-14  7:20   ` Cong Wang
@ 2021-01-14 17:06     ` Jakub Kicinski
  0 siblings, 0 replies; 8+ messages in thread
From: Jakub Kicinski @ 2021-01-14 17:06 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, Cong Wang, syzbot,
	Jamal Hadi Salim, Xin Long, Jiri Pirko

On Wed, 13 Jan 2021 23:20:24 -0800 Cong Wang wrote:
> On Tue, Jan 12, 2021 at 5:38 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Mon, 11 Jan 2021 18:55:48 -0800 Cong Wang wrote:  
> > > 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: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> > > 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 <cong.wang@bytedance.com>  
> >
> > 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()...

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-12  2:55 [Patch net] cls_flower: call nla_ok() before nla_next() Cong Wang
2021-01-12 11:52 ` Xin Long
2021-01-12 17:43   ` Cong Wang
2021-01-13  2:22     ` Xin Long
2021-01-14  7:21       ` Cong Wang
2021-01-13  1:38 ` Jakub Kicinski
2021-01-14  7:20   ` Cong Wang
2021-01-14 17:06     ` Jakub Kicinski

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git