netdev - Re: [PATCH 3/9] sctp: Check address length before reading srx

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]

Date:   Fri, 12 Apr 2019 12:48:15 -0400
From:   Neil Horman <nhorman@...driver.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Vlad Yasevich <vyasevich@...il.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH 3/9] sctp: Check address length before reading
 srx_service field

On Fri, Apr 12, 2019 at 08:21:26PM +0900, Tetsuo Handa wrote:
> On 2019/04/12 20:12, Neil Horman wrote:
> > On Fri, Apr 12, 2019 at 07:53:10PM +0900, Tetsuo Handa wrote:
> >> KMSAN will complain if valid address length passed to connect() is shorter
> >> than sizeof("struct sockaddr"->sa_family) bytes.
> >>
> >> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> >> ---
> >>  net/sctp/socket.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> >> index 9874e60c9b0d..4583fa914e62 100644
> >> --- a/net/sctp/socket.c
> >> +++ b/net/sctp/socket.c
> >> @@ -4847,7 +4847,8 @@ static int sctp_connect(struct sock *sk, struct sockaddr *addr,
> >>  	}
> >>  
> >>  	/* Validate addr_len before calling common connect/connectx routine. */
> >> -	af = sctp_get_af_specific(addr->sa_family);
> >> +	af = addr_len < offsetofend(struct sockaddr, sa_family) ? NULL :
> >> +		sctp_get_af_specific(addr->sa_family);
> > I don't see anything wrong with the patch per se, but sctp_get_af_specific will
> > always return a value that is greater than offsetofend(struct sockaddr,
> > sa_family).  So while this patch doesn't do anything wrong, it seems your fixing
> > an erroneous KMSAN warning in the SCTP code.  Shouldn't KMSAN just be made to
> > not complain if the address length is valid?
> 
> KMSAN does not complain if the address length is valid. KMSAN complains
> at addr->sa_family if the address length passed to connect() is either 0 or 1.
> Thus, we need to verify addr_len >= 2 before accessing addr->sa_family.
> 
Ah, ok, then with the subject change you note in your other email
Acked-by: Neil Horman <nhorman@...driver.com>

> > 
> > Neil
> > 
> >>  	if (!af || addr_len < af->sockaddr_len) {
> >>  		err = -EINVAL;
> >>  	} else {
> >> -- 
> >> 2.16.5
> >>
> >>
> > 
> 
>