netdev - Re: [Patch net] net_sched: add max len check for TCA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <36471b0d-cc83-40aa-3ded-39e864dcceb0@gmail.com>
Date:   Wed, 18 Sep 2019 20:41:43 -0600
From:   David Ahern <dsahern@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
Cc:     syzbot+618aacd49e8c8b8486bd@...kaller.appspotmail.com,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [Patch net] net_sched: add max len check for TCA_KIND

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@...kaller.appspotmail.com

What is the actual sysbot report?

> Cc: David Ahern <dsahern@...il.com>
> Cc: Jamal Hadi Salim <jhs@...atatu.com>
> Cc: Jiri Pirko <jiri@...nulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.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@...il.com>