| Message ID | 1555066390-9403-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/9] net/rds: Check address length before reading address family | expand |
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 > >
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 >> >> >
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 {
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 > >> > >> > > > >
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.
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 {
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(-)