All of lore.kernel.org
 help / color / mirror / Atom feed
* [Patch net] net_sched: add max len check for TCA_KIND
@ 2019-09-18 23:24 Cong Wang
  2019-09-19  2:41 ` David Ahern
  2019-09-19  6:40 ` Jiri Pirko
  0 siblings, 2 replies; 9+ messages in thread
From: Cong Wang @ 2019-09-18 23:24 UTC (permalink / raw)
  To: netdev
  Cc: Cong Wang, syzbot+618aacd49e8c8b8486bd, David Ahern,
	Jamal Hadi Salim, Jiri Pirko

The TCA_KIND attribute is of NLA_STRING which does not check
the NUL char. KMSAN reported an uninit-value of TCA_KIND which
is likely caused by the lack of NUL.

Change it to NLA_NUL_STRING and add a max len too.

Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
Reported-and-tested-by: syzbot+618aacd49e8c8b8486bd@syzkaller.appspotmail.com
Cc: David Ahern <dsahern@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_api.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 1047825d9f48..81d58b280612 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1390,7 +1390,8 @@ check_loop_fn(struct Qdisc *q, unsigned long cl, struct qdisc_walker *w)
 }
 
 const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
-	[TCA_KIND]		= { .type = NLA_STRING },
+	[TCA_KIND]		= { .type = NLA_NUL_STRING,
+				    .len = IFNAMSIZ - 1 },
 	[TCA_RATE]		= { .type = NLA_BINARY,
 				    .len = sizeof(struct tc_estimator) },
 	[TCA_STAB]		= { .type = NLA_NESTED },
-- 
2.21.0


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

* Re: [Patch net] net_sched: add max len check for TCA_KIND
  2019-09-18 23:24 [Patch net] net_sched: add max len check for TCA_KIND Cong Wang
@ 2019-09-19  2:41 ` David Ahern
  2019-09-19  5:15   ` Cong Wang
  2019-09-19  6:40 ` Jiri Pirko
  1 sibling, 1 reply; 9+ messages in thread
From: David Ahern @ 2019-09-19  2:41 UTC (permalink / raw)
  To: Cong Wang, netdev
  Cc: syzbot+618aacd49e8c8b8486bd, Jamal Hadi Salim, Jiri Pirko

On 9/18/19 5:24 PM, Cong Wang wrote:
> The TCA_KIND attribute is of NLA_STRING which does not check
> the NUL char. KMSAN reported an uninit-value of TCA_KIND which
> is likely caused by the lack of NUL.
> 
> Change it to NLA_NUL_STRING and add a max len too.
> 
> Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")

The commit referenced here did not introduce the ability to go beyond
memory boundaries with string comparisons. Rather, it was not complete
solution for attribute validation. I say that wrt to the fix getting
propagated to the correct stable releases.

> Reported-and-tested-by: syzbot+618aacd49e8c8b8486bd@syzkaller.appspotmail.com

What is the actual sysbot report?

> Cc: David Ahern <dsahern@gmail.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/sched/sch_api.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 1047825d9f48..81d58b280612 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1390,7 +1390,8 @@ check_loop_fn(struct Qdisc *q, unsigned long cl, struct qdisc_walker *w)
>  }
>  
>  const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
> -	[TCA_KIND]		= { .type = NLA_STRING },
> +	[TCA_KIND]		= { .type = NLA_NUL_STRING,
> +				    .len = IFNAMSIZ - 1 },
>  	[TCA_RATE]		= { .type = NLA_BINARY,
>  				    .len = sizeof(struct tc_estimator) },
>  	[TCA_STAB]		= { .type = NLA_NESTED },
> 

This is a better policy so for that:
Reviewed-by: David Ahern <dsahern@gmail.com>

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

* Re: [Patch net] net_sched: add max len check for TCA_KIND
  2019-09-19  2:41 ` David Ahern
@ 2019-09-19  5:15   ` Cong Wang
  2019-09-22  2:24     ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Cong Wang @ 2019-09-19  5:15 UTC (permalink / raw)
  To: David Ahern
  Cc: Linux Kernel Network Developers, syzbot, Jamal Hadi Salim, Jiri Pirko

On Wed, Sep 18, 2019 at 7:41 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 9/18/19 5:24 PM, Cong Wang wrote:
> > The TCA_KIND attribute is of NLA_STRING which does not check
> > the NUL char. KMSAN reported an uninit-value of TCA_KIND which
> > is likely caused by the lack of NUL.
> >
> > Change it to NLA_NUL_STRING and add a max len too.
> >
> > Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
>
> The commit referenced here did not introduce the ability to go beyond
> memory boundaries with string comparisons. Rather, it was not complete
> solution for attribute validation. I say that wrt to the fix getting
> propagated to the correct stable releases.

I think this patch should be backported to wherever commit 8b4c3cdd9dd8
goes, this is why I picked it as Fixes.

>
> > Reported-and-tested-by: syzbot+618aacd49e8c8b8486bd@syzkaller.appspotmail.com
>
> What is the actual sysbot report?

https://marc.info/?l=linux-kernel&m=156862916112881&w=2

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

* Re: [Patch net] net_sched: add max len check for TCA_KIND
  2019-09-18 23:24 [Patch net] net_sched: add max len check for TCA_KIND Cong Wang
  2019-09-19  2:41 ` David Ahern
@ 2019-09-19  6:40 ` Jiri Pirko
  1 sibling, 0 replies; 9+ messages in thread
From: Jiri Pirko @ 2019-09-19  6:40 UTC (permalink / raw)
  To: Cong Wang
  Cc: netdev, syzbot+618aacd49e8c8b8486bd, David Ahern, Jamal Hadi Salim

Thu, Sep 19, 2019 at 01:24:12AM CEST, xiyou.wangcong@gmail.com wrote:
>The TCA_KIND attribute is of NLA_STRING which does not check
>the NUL char. KMSAN reported an uninit-value of TCA_KIND which
>is likely caused by the lack of NUL.
>
>Change it to NLA_NUL_STRING and add a max len too.
>
>Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
>Reported-and-tested-by: syzbot+618aacd49e8c8b8486bd@syzkaller.appspotmail.com

Acked-by: Jiri Pirko <jiri@mellanox.com>

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

* Re: [Patch net] net_sched: add max len check for TCA_KIND
  2019-09-19  5:15   ` Cong Wang
@ 2019-09-22  2:24     ` Jakub Kicinski
  2019-10-03 19:45       ` Marcelo Ricardo Leitner
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2019-09-22  2:24 UTC (permalink / raw)
  To: Cong Wang
  Cc: David Ahern, Linux Kernel Network Developers, syzbot,
	Jamal Hadi Salim, Jiri Pirko

On Wed, 18 Sep 2019 22:15:24 -0700, Cong Wang wrote:
> On Wed, Sep 18, 2019 at 7:41 PM David Ahern <dsahern@gmail.com> wrote:
> > On 9/18/19 5:24 PM, Cong Wang wrote:  
> > > The TCA_KIND attribute is of NLA_STRING which does not check
> > > the NUL char. KMSAN reported an uninit-value of TCA_KIND which
> > > is likely caused by the lack of NUL.
> > >
> > > Change it to NLA_NUL_STRING and add a max len too.
> > >
> > > Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")  
> >
> > The commit referenced here did not introduce the ability to go beyond
> > memory boundaries with string comparisons. Rather, it was not complete
> > solution for attribute validation. I say that wrt to the fix getting
> > propagated to the correct stable releases.  
> 
> I think this patch should be backported to wherever commit 8b4c3cdd9dd8
> goes, this is why I picked it as Fixes.

Applied, queued for 4.14+, thanks!

> >  
> > > Reported-and-tested-by: syzbot+618aacd49e8c8b8486bd@syzkaller.appspotmail.com  
> >
> > What is the actual sysbot report?  
> 
> https://marc.info/?l=linux-kernel&m=156862916112881&w=2


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

* Re: [Patch net] net_sched: add max len check for TCA_KIND
  2019-09-22  2:24     ` Jakub Kicinski
@ 2019-10-03 19:45       ` Marcelo Ricardo Leitner
  2019-10-04 22:54         ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-10-03 19:45 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Cong Wang, David Ahern, Linux Kernel Network Developers, syzbot,
	Jamal Hadi Salim, Jiri Pirko

On Sat, Sep 21, 2019 at 07:24:34PM -0700, Jakub Kicinski wrote:
> On Wed, 18 Sep 2019 22:15:24 -0700, Cong Wang wrote:
> > On Wed, Sep 18, 2019 at 7:41 PM David Ahern <dsahern@gmail.com> wrote:
> > > On 9/18/19 5:24 PM, Cong Wang wrote:  
> > > > The TCA_KIND attribute is of NLA_STRING which does not check
> > > > the NUL char. KMSAN reported an uninit-value of TCA_KIND which
> > > > is likely caused by the lack of NUL.
> > > >
> > > > Change it to NLA_NUL_STRING and add a max len too.
> > > >
> > > > Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")  
> > >
> > > The commit referenced here did not introduce the ability to go beyond
> > > memory boundaries with string comparisons. Rather, it was not complete
> > > solution for attribute validation. I say that wrt to the fix getting
> > > propagated to the correct stable releases.  
> > 
> > I think this patch should be backported to wherever commit 8b4c3cdd9dd8
> > goes, this is why I picked it as Fixes.
> 
> Applied, queued for 4.14+, thanks!

Ahm, this breaks some user applications.

I'm getting "Attribute failed policy validation" extack error while
adding ingress qdisc on an app using libmnl, because it just doesn't
pack the null byte there if it uses mnl_attr_put_str():
https://git.netfilter.org/libmnl/tree/src/attr.c#n481
Unless it uses mnl_attr_put_strz() instead.

Though not sure who's to blame here, as one could argue that the
app should have been using the latter in the first place, but well..
it worked and produced the right results.

Ditto for 199ce850ce11 ("net_sched: add policy validation for action
attributes") on TCA_ACT_KIND.

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

* Re: [Patch net] net_sched: add max len check for TCA_KIND
  2019-10-03 19:45       ` Marcelo Ricardo Leitner
@ 2019-10-04 22:54         ` Jakub Kicinski
  2019-10-04 23:23           ` Cong Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2019-10-04 22:54 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner, Cong Wang
  Cc: David Ahern, Linux Kernel Network Developers, syzbot,
	Jamal Hadi Salim, Jiri Pirko

On Thu, 3 Oct 2019 16:45:25 -0300, Marcelo Ricardo Leitner wrote:
> On Sat, Sep 21, 2019 at 07:24:34PM -0700, Jakub Kicinski wrote:
> > Applied, queued for 4.14+, thanks!  
> 
> Ahm, this breaks some user applications.
> 
> I'm getting "Attribute failed policy validation" extack error while
> adding ingress qdisc on an app using libmnl, because it just doesn't
> pack the null byte there if it uses mnl_attr_put_str():
> https://git.netfilter.org/libmnl/tree/src/attr.c#n481
> Unless it uses mnl_attr_put_strz() instead.
> 
> Though not sure who's to blame here, as one could argue that the
> app should have been using the latter in the first place, but well..
> it worked and produced the right results.
> 
> Ditto for 199ce850ce11 ("net_sched: add policy validation for action
> attributes") on TCA_ACT_KIND.

Thanks for the report Marcelo! This netlink validation stuff is always
super risky I figured better find out if something breaks sooner than
later, hence the backport.

So if I'm understanding this would be the fix?

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 2558f00f6b3e..bcc1178ce50d 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -832,8 +832,7 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
 }
 
 static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
-       [TCA_ACT_KIND]          = { .type = NLA_NUL_STRING,
-                                   .len = IFNAMSIZ - 1 },
+       [TCA_ACT_KIND]          = { .type = NLA_STRING },
        [TCA_ACT_INDEX]         = { .type = NLA_U32 },
        [TCA_ACT_COOKIE]        = { .type = NLA_BINARY,
                                    .len = TC_COOKIE_MAX_SIZE },
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 81d58b280612..1047825d9f48 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1390,8 +1390,7 @@ check_loop_fn(struct Qdisc *q, unsigned long cl, struct qdisc_walker *w)
 }
 
 const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
-       [TCA_KIND]              = { .type = NLA_NUL_STRING,
-                                   .len = IFNAMSIZ - 1 },
+       [TCA_KIND]              = { .type = NLA_STRING },
        [TCA_RATE]              = { .type = NLA_BINARY,
                                    .len = sizeof(struct tc_estimator) },
        [TCA_STAB]              = { .type = NLA_NESTED },


Cong, are you planning to take a look? With this we have to find a
different way to deal with the KMSAN report you mentioned :(

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

* Re: [Patch net] net_sched: add max len check for TCA_KIND
  2019-10-04 22:54         ` Jakub Kicinski
@ 2019-10-04 23:23           ` Cong Wang
  2019-10-04 23:47             ` Jakub Kicinski
  0 siblings, 1 reply; 9+ messages in thread
From: Cong Wang @ 2019-10-04 23:23 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Marcelo Ricardo Leitner, David Ahern,
	Linux Kernel Network Developers, syzbot, Jamal Hadi Salim,
	Jiri Pirko

On Fri, Oct 4, 2019 at 3:54 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu, 3 Oct 2019 16:45:25 -0300, Marcelo Ricardo Leitner wrote:
> > On Sat, Sep 21, 2019 at 07:24:34PM -0700, Jakub Kicinski wrote:
> > > Applied, queued for 4.14+, thanks!
> >
> > Ahm, this breaks some user applications.
> >
> > I'm getting "Attribute failed policy validation" extack error while
> > adding ingress qdisc on an app using libmnl, because it just doesn't
> > pack the null byte there if it uses mnl_attr_put_str():
> > https://git.netfilter.org/libmnl/tree/src/attr.c#n481
> > Unless it uses mnl_attr_put_strz() instead.
> >
> > Though not sure who's to blame here, as one could argue that the
> > app should have been using the latter in the first place, but well..
> > it worked and produced the right results.
> >
> > Ditto for 199ce850ce11 ("net_sched: add policy validation for action
> > attributes") on TCA_ACT_KIND.
>
> Thanks for the report Marcelo! This netlink validation stuff is always
> super risky I figured better find out if something breaks sooner than
> later, hence the backport.
>
> So if I'm understanding this would be the fix?

Of course not, you just break KMSAN again. Please read the original
report.

I will send a patch to use nla_strlcpy() instead, I think it will make
everyone happy here.

Thanks.

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

* Re: [Patch net] net_sched: add max len check for TCA_KIND
  2019-10-04 23:23           ` Cong Wang
@ 2019-10-04 23:47             ` Jakub Kicinski
  0 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2019-10-04 23:47 UTC (permalink / raw)
  To: Cong Wang
  Cc: Marcelo Ricardo Leitner, David Ahern,
	Linux Kernel Network Developers, syzbot, Jamal Hadi Salim,
	Jiri Pirko

On Fri, 4 Oct 2019 16:23:40 -0700, Cong Wang wrote:
> On Fri, Oct 4, 2019 at 3:54 PM Jakub Kicinski wrote:
> > On Thu, 3 Oct 2019 16:45:25 -0300, Marcelo Ricardo Leitner wrote:  
> > > On Sat, Sep 21, 2019 at 07:24:34PM -0700, Jakub Kicinski wrote:  
> > > > Applied, queued for 4.14+, thanks!  
> > >
> > > Ahm, this breaks some user applications.
> > >
> > > I'm getting "Attribute failed policy validation" extack error while
> > > adding ingress qdisc on an app using libmnl, because it just doesn't
> > > pack the null byte there if it uses mnl_attr_put_str():
> > > https://git.netfilter.org/libmnl/tree/src/attr.c#n481
> > > Unless it uses mnl_attr_put_strz() instead.
> > >
> > > Though not sure who's to blame here, as one could argue that the
> > > app should have been using the latter in the first place, but well..
> > > it worked and produced the right results.
> > >
> > > Ditto for 199ce850ce11 ("net_sched: add policy validation for action
> > > attributes") on TCA_ACT_KIND.  
> >
> > Thanks for the report Marcelo! This netlink validation stuff is always
> > super risky I figured better find out if something breaks sooner than
> > later, hence the backport.
> >
> > So if I'm understanding this would be the fix?  
> 
> Of course not, you just break KMSAN again. Please read the original
> report.

The fix for the regression. I'm establishing the rest of 199ce850ce11
("net_sched: add policy validation for action attributes") is fine.

I mentioned this brings back the problem KMSAN reported in the part of
the email you cut off. str*cpy is the obvious answer for reimplementing
that fix.

> I will send a patch

Please do.

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

end of thread, other threads:[~2019-10-04 23:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-18 23:24 [Patch net] net_sched: add max len check for TCA_KIND Cong Wang
2019-09-19  2:41 ` David Ahern
2019-09-19  5:15   ` Cong Wang
2019-09-22  2:24     ` Jakub Kicinski
2019-10-03 19:45       ` Marcelo Ricardo Leitner
2019-10-04 22:54         ` Jakub Kicinski
2019-10-04 23:23           ` Cong Wang
2019-10-04 23:47             ` Jakub Kicinski
2019-09-19  6:40 ` Jiri Pirko

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.