* [PATCH 3/9] sctp: Check address length before reading srx_service field
@ 2019-04-12 10:53 Tetsuo Handa
2019-04-12 11:12 ` Neil Horman
2019-04-12 17:25 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Tetsuo Handa @ 2019-04-12 10:53 UTC (permalink / raw)
To: Vlad Yasevich, Neil Horman, Marcelo Ricardo Leitner
Cc: David S. Miller, netdev, Tetsuo Handa
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);
if (!af || addr_len < af->sockaddr_len) {
err = -EINVAL;
} else {
--
2.16.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/9] sctp: Check address length before reading srx_service field
2019-04-12 10:53 [PATCH 3/9] sctp: Check address length before reading srx_service field Tetsuo Handa
@ 2019-04-12 11:12 ` Neil Horman
2019-04-12 11:21 ` Tetsuo Handa
2019-04-12 17:25 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: Neil Horman @ 2019-04-12 11:12 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Vlad Yasevich, Marcelo Ricardo Leitner, David S. Miller, netdev
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?
Neil
> if (!af || addr_len < af->sockaddr_len) {
> err = -EINVAL;
> } else {
> --
> 2.16.5
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/9] sctp: Check address length before reading srx_service field
2019-04-12 11:12 ` Neil Horman
@ 2019-04-12 11:21 ` Tetsuo Handa
2019-04-12 12:38 ` Tetsuo Handa
2019-04-12 16:48 ` Neil Horman
0 siblings, 2 replies; 6+ messages in thread
From: Tetsuo Handa @ 2019-04-12 11:21 UTC (permalink / raw)
To: Neil Horman
Cc: Vlad Yasevich, Marcelo Ricardo Leitner, David S. Miller, netdev
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
>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/9] sctp: Check address length before reading srx_service field
2019-04-12 11:21 ` Tetsuo Handa
@ 2019-04-12 12:38 ` Tetsuo Handa
2019-04-12 16:48 ` Neil Horman
1 sibling, 0 replies; 6+ messages in thread
From: Tetsuo Handa @ 2019-04-12 12:38 UTC (permalink / raw)
To: Neil Horman
Cc: Vlad Yasevich, Marcelo Ricardo Leitner, David S. Miller, netdev
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-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.
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-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);
if (!af || addr_len < af->sockaddr_len) {
err = -EINVAL;
} else {
--
2.16.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/9] sctp: Check address length before reading srx_service field
2019-04-12 11:21 ` Tetsuo Handa
2019-04-12 12:38 ` Tetsuo Handa
@ 2019-04-12 16:48 ` Neil Horman
1 sibling, 0 replies; 6+ messages in thread
From: Neil Horman @ 2019-04-12 16:48 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Vlad Yasevich, Marcelo Ricardo Leitner, David S. Miller, netdev
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@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.
>
Ah, ok, then with the subject change you note in your other email
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> >
> > Neil
> >
> >> if (!af || addr_len < af->sockaddr_len) {
> >> err = -EINVAL;
> >> } else {
> >> --
> >> 2.16.5
> >>
> >>
> >
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/9] sctp: Check address length before reading srx_service field
2019-04-12 10:53 [PATCH 3/9] sctp: Check address length before reading srx_service field Tetsuo Handa
2019-04-12 11:12 ` Neil Horman
@ 2019-04-12 17:25 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2019-04-12 17:25 UTC (permalink / raw)
To: penguin-kernel; +Cc: vyasevich, nhorman, marcelo.leitner, netdev
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri, 12 Apr 2019 19:53:10 +0900
> 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>
Applied with Subject fixed.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-12 17:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-12 10:53 [PATCH 3/9] sctp: Check address length before reading srx_service field Tetsuo Handa
2019-04-12 11:12 ` Neil Horman
2019-04-12 11:21 ` Tetsuo Handa
2019-04-12 12:38 ` Tetsuo Handa
2019-04-12 16:48 ` Neil Horman
2019-04-12 17:25 ` David Miller
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.