Re: [PATCH 3/9] sctp: Check address length before reading srx_service field

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

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@I-love.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.

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