linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] [PATCH] net: fix uninit value error in __sys_sendmmsg
@ 2020-09-13  5:56 Anant Thazhemadam
  2020-09-13  6:13 ` Greg KH
  0 siblings, 1 reply; 5+ messages in thread
From: Anant Thazhemadam @ 2020-09-13  5:56 UTC (permalink / raw)
  Cc: Anant Thazhemadam, netdev, linux-kernel, Jakub Kicinski,
	syzbot+09a5d591c1f98cf5efcb, David S. Miller,
	linux-kernel-mentees

The crash report showed that there was a local variable;

----iovstack.i@__sys_sendmmsg created at:
 ___sys_sendmsg net/socket.c:2388 [inline]
 __sys_sendmmsg+0x6db/0xc90 net/socket.c:2480
 
 that was left uninitialized.

The contents of iovstack are of interest, since the respective pointer
is passed down as an argument to sendmsg_copy_msghdr as well.
Initializing this contents of this stack prevents this bug from happening.

Since the memory that was initialized is freed at the end of the function
call, memory leaks are not likely to be an issue.

syzbot seems to have triggered this error by passing an array of 0's as
a parameter while making the initial system call.

Reported-by: syzbot+09a5d591c1f98cf5efcb@syzkaller.appspotmail.com
Tested-by: syzbot+09a5d591c1f98cf5efcb@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/socket.c b/net/socket.c
index 0c0144604f81..d74443dfd73b 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2396,6 +2396,7 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
 {
 	struct sockaddr_storage address;
 	struct iovec iovstack[UIO_FASTIOV], *iov = iovstack;
+	memset(iov, 0, UIO_FASTIOV);
 	ssize_t err;
 
 	msg_sys->msg_name = &address;
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] net: fix uninit value error in __sys_sendmmsg
  2020-09-13  5:56 [Linux-kernel-mentees] [PATCH] net: fix uninit value error in __sys_sendmmsg Anant Thazhemadam
@ 2020-09-13  6:13 ` Greg KH
  2020-09-13  6:20   ` Anant Thazhemadam
  2020-09-14  7:58   ` David Laight
  0 siblings, 2 replies; 5+ messages in thread
From: Greg KH @ 2020-09-13  6:13 UTC (permalink / raw)
  To: Anant Thazhemadam
  Cc: netdev, linux-kernel, Jakub Kicinski,
	syzbot+09a5d591c1f98cf5efcb, David S. Miller,
	linux-kernel-mentees

On Sun, Sep 13, 2020 at 11:26:39AM +0530, Anant Thazhemadam wrote:
> The crash report showed that there was a local variable;
> 
> ----iovstack.i@__sys_sendmmsg created at:
>  ___sys_sendmsg net/socket.c:2388 [inline]
>  __sys_sendmmsg+0x6db/0xc90 net/socket.c:2480
>  
>  that was left uninitialized.
> 
> The contents of iovstack are of interest, since the respective pointer
> is passed down as an argument to sendmsg_copy_msghdr as well.
> Initializing this contents of this stack prevents this bug from happening.
> 
> Since the memory that was initialized is freed at the end of the function
> call, memory leaks are not likely to be an issue.
> 
> syzbot seems to have triggered this error by passing an array of 0's as
> a parameter while making the initial system call.
> 
> Reported-by: syzbot+09a5d591c1f98cf5efcb@syzkaller.appspotmail.com
> Tested-by: syzbot+09a5d591c1f98cf5efcb@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> ---
>  net/socket.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/socket.c b/net/socket.c
> index 0c0144604f81..d74443dfd73b 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2396,6 +2396,7 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
>  {
>  	struct sockaddr_storage address;
>  	struct iovec iovstack[UIO_FASTIOV], *iov = iovstack;
> +	memset(iov, 0, UIO_FASTIOV);
>  	ssize_t err;
>  
>  	msg_sys->msg_name = &address;

I don't think you built this code change, otherwise you would have seen
that it adds a build warning to the system, right?

:(
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] net: fix uninit value error in __sys_sendmmsg
  2020-09-13  6:13 ` Greg KH
@ 2020-09-13  6:20   ` Anant Thazhemadam
  2020-09-13 21:25     ` David Miller
  2020-09-14  7:58   ` David Laight
  1 sibling, 1 reply; 5+ messages in thread
From: Anant Thazhemadam @ 2020-09-13  6:20 UTC (permalink / raw)
  To: Greg KH
  Cc: netdev, linux-kernel, Jakub Kicinski,
	syzbot+09a5d591c1f98cf5efcb, David S. Miller,
	linux-kernel-mentees


On 13/09/20 11:43 am, Greg KH wrote:
> On Sun, Sep 13, 2020 at 11:26:39AM +0530, Anant Thazhemadam wrote:
>> The crash report showed that there was a local variable;
>>
>> ----iovstack.i@__sys_sendmmsg created at:
>>  ___sys_sendmsg net/socket.c:2388 [inline]
>>  __sys_sendmmsg+0x6db/0xc90 net/socket.c:2480
>>  
>>  that was left uninitialized.
>>
>> The contents of iovstack are of interest, since the respective pointer
>> is passed down as an argument to sendmsg_copy_msghdr as well.
>> Initializing this contents of this stack prevents this bug from happening.
>>
>> Since the memory that was initialized is freed at the end of the function
>> call, memory leaks are not likely to be an issue.
>>
>> syzbot seems to have triggered this error by passing an array of 0's as
>> a parameter while making the initial system call.
>>
>> Reported-by: syzbot+09a5d591c1f98cf5efcb@syzkaller.appspotmail.com
>> Tested-by: syzbot+09a5d591c1f98cf5efcb@syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
>> ---
>>  net/socket.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/net/socket.c b/net/socket.c
>> index 0c0144604f81..d74443dfd73b 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -2396,6 +2396,7 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
>>  {
>>  	struct sockaddr_storage address;
>>  	struct iovec iovstack[UIO_FASTIOV], *iov = iovstack;
>> +	memset(iov, 0, UIO_FASTIOV);
>>  	ssize_t err;
>>  
>>  	msg_sys->msg_name = &address;
> I don't think you built this code change, otherwise you would have seen
> that it adds a build warning to the system, right?
>
> :(
My apologies. I think I ended up overlooking the build warning. Thank you for pointing that out.
If everything else looks good, I'd be happy to send in a v2 that fixes this build warning.

Thanks,
Anant
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] net: fix uninit value error in __sys_sendmmsg
  2020-09-13  6:20   ` Anant Thazhemadam
@ 2020-09-13 21:25     ` David Miller
  0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2020-09-13 21:25 UTC (permalink / raw)
  To: anant.thazhemadam
  Cc: greg, linux-kernel, syzbot+09a5d591c1f98cf5efcb, netdev, kuba,
	linux-kernel-mentees

From: Anant Thazhemadam <anant.thazhemadam@gmail.com>
Date: Sun, 13 Sep 2020 11:50:52 +0530

> My apologies. I think I ended up overlooking the build warning.

You "think" you overlooked the build warning?  You don't actually
know?

If you aren't willing to even make sure the build is clean after your
changes, why should we be willing to review and integrate your changes?

This kind of carelessness costs other developers their valuable time,
please treat it with more respect than you have.

Thank you.

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] net: fix uninit value error in __sys_sendmmsg
  2020-09-13  6:13 ` Greg KH
  2020-09-13  6:20   ` Anant Thazhemadam
@ 2020-09-14  7:58   ` David Laight
  1 sibling, 0 replies; 5+ messages in thread
From: David Laight @ 2020-09-14  7:58 UTC (permalink / raw)
  To: 'Greg KH', Anant Thazhemadam
  Cc: netdev, linux-kernel, Jakub Kicinski,
	syzbot+09a5d591c1f98cf5efcb, David S. Miller,
	linux-kernel-mentees

From: Greg KH
> Sent: 13 September 2020 07:14
> On Sun, Sep 13, 2020 at 11:26:39AM +0530, Anant Thazhemadam wrote:
> > The crash report showed that there was a local variable;
> >
> > ----iovstack.i@__sys_sendmmsg created at:
> >  ___sys_sendmsg net/socket.c:2388 [inline]
> >  __sys_sendmmsg+0x6db/0xc90 net/socket.c:2480
> >
> >  that was left uninitialized.
> >
> > The contents of iovstack are of interest, since the respective pointer
> > is passed down as an argument to sendmsg_copy_msghdr as well.
> > Initializing this contents of this stack prevents this bug from happening.
> >
> > Since the memory that was initialized is freed at the end of the function
> > call, memory leaks are not likely to be an issue.
> >
> > syzbot seems to have triggered this error by passing an array of 0's as
> > a parameter while making the initial system call.
> >
> > Reported-by: syzbot+09a5d591c1f98cf5efcb@syzkaller.appspotmail.com
> > Tested-by: syzbot+09a5d591c1f98cf5efcb@syzkaller.appspotmail.com
> > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> > ---
> >  net/socket.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/net/socket.c b/net/socket.c
> > index 0c0144604f81..d74443dfd73b 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -2396,6 +2396,7 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
> >  {
> >  	struct sockaddr_storage address;
> >  	struct iovec iovstack[UIO_FASTIOV], *iov = iovstack;
> > +	memset(iov, 0, UIO_FASTIOV);
> >  	ssize_t err;
> >
> >  	msg_sys->msg_name = &address;
> 
> I don't think you built this code change, otherwise you would have seen
> that it adds a build warning to the system, right?

Also it can't be the right 'fix' for whatever sysbot found.
(I can't find the sysbot report.)

Zeroing iov[] just slows down a path that is already too slow because
of the contorted functions used to read in iov[].

If it does need to be zerod then it would be needed in a lot
of other code paths that read in iov[].

If a zero length iov[] needs converting into a single entity
with a zero length - then that needs to be done elsewhere.

I've a patch series I might redo that changes the code that
reads in iov[] to return the address of any buffer that
needed to be malloced (more than UIV_FASTIO buffers) rather
than using the iov parameter to pass in the cache and
return the buffer to free.
It would be less confusing and error prone.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-09-14  7:59 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-13  5:56 [Linux-kernel-mentees] [PATCH] net: fix uninit value error in __sys_sendmmsg Anant Thazhemadam
2020-09-13  6:13 ` Greg KH
2020-09-13  6:20   ` Anant Thazhemadam
2020-09-13 21:25     ` David Miller
2020-09-14  7:58   ` David Laight

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).