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

[Tetsuo Handa]

On 2019/04/12 20:21, 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-ker...@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.

I realized that this patch's subject was wrong. The subject should be:

[PATCH 3/9] sctp: Check address length before reading address family

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-ker...@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);
        if (!af || addr_len < af->sockaddr_len) {
                err = -EINVAL;
        } else {
-- 
2.16.5