linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] [PATCH v2] net: fix uninit value error in __sys_sendmmsg
@ 2020-09-13 11:03 Anant Thazhemadam
  2020-09-13 21:39 ` David Miller
  2020-09-14  8:14 ` Anant Thazhemadam
  0 siblings, 2 replies; 3+ messages in thread
From: Anant Thazhemadam @ 2020-09-13 11:03 UTC (permalink / raw)
  Cc: Anant Thazhemadam, netdev, linux-kernel, Jakub Kicinski,
	syzbot+09a5d591c1f98cf5efcb, David S. Miller,
	linux-kernel-mentees

The crash report indicated 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.

Initializing this stack to 0s prevents this bug from happening.
Since the memory pointed to by *iov 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 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>
---
Changes from v1:
	* Fixed the build warning that v1 had introduced
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/socket.c b/net/socket.c
index 0c0144604f81..1e6f9b54982c 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2398,6 +2398,7 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
 	struct iovec iovstack[UIO_FASTIOV], *iov = iovstack;
 	ssize_t err;
 
+	memset(iov, 0, UIO_FASTIOV);
 	msg_sys->msg_name = &address;
 
 	err = sendmsg_copy_msghdr(msg_sys, msg, flags, &iov);
-- 
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] 3+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] net: fix uninit value error in __sys_sendmmsg
  2020-09-13 11:03 [Linux-kernel-mentees] [PATCH v2] net: fix uninit value error in __sys_sendmmsg Anant Thazhemadam
@ 2020-09-13 21:39 ` David Miller
  2020-09-14  8:14 ` Anant Thazhemadam
  1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2020-09-13 21:39 UTC (permalink / raw)
  To: anant.thazhemadam
  Cc: syzbot+09a5d591c1f98cf5efcb, netdev, linux-kernel-mentees,
	linux-kernel, kuba

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

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

Did you even test this?

Seriously?

UIO_FASTIOV is the number of entries in 'iovstack', it's not the
size with would be "UIO_FASTIOV * sizeof (struct iovec)", or
even "sizeof(iovstack)"

So could you really explain to me how you tested this patch for
correctness, and for any functional or performance regressions
that may occur?

Because, once you correct that size argument to memset() we will now
have a huge memset() for _EVERY_ _SINGLE_ sendmsg() done by the
system.  And that will cause severe performance regressions for many
workloads involving networking.

This patch submission has been extremely careless on so many levels. I
sincerely wish you would take your time with these changes and not be
so lacking in the areas of testing and validation.

It is always a reg flag when a submitter doesn't even notice an
obvious compiler warning that reviewers like Greg and myself can see
even without trying to build your code changes.

_______________________________________________
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] 3+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] net: fix uninit value error in __sys_sendmmsg
  2020-09-13 11:03 [Linux-kernel-mentees] [PATCH v2] net: fix uninit value error in __sys_sendmmsg Anant Thazhemadam
  2020-09-13 21:39 ` David Miller
@ 2020-09-14  8:14 ` Anant Thazhemadam
  1 sibling, 0 replies; 3+ messages in thread
From: Anant Thazhemadam @ 2020-09-14  8:14 UTC (permalink / raw)
  Cc: netdev, linux-kernel, Jakub Kicinski,
	syzbot+09a5d591c1f98cf5efcb, David S. Miller,
	linux-kernel-mentees


On 13/09/20 4:33 pm, Anant Thazhemadam wrote:
> The crash report indicated 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.
>
> Initializing this stack to 0s prevents this bug from happening.
> Since the memory pointed to by *iov 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 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>
> ---
> Changes from v1:
> 	* Fixed the build warning that v1 had introduced
>  net/socket.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/socket.c b/net/socket.c
> index 0c0144604f81..1e6f9b54982c 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2398,6 +2398,7 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
>  	struct iovec iovstack[UIO_FASTIOV], *iov = iovstack;
>  	ssize_t err;
>  
> +	memset(iov, 0, UIO_FASTIOV);
>  	msg_sys->msg_name = &address;
>  
>  	err = sendmsg_copy_msghdr(msg_sys, msg, flags, &iov);
It has since been determined that this patch is incorrect, and even if it were correct,
provides a huge performance overhead, that is not welcome. Kindly ignore this patch.
Sorry.

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] 3+ messages in thread

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

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-13 11:03 [Linux-kernel-mentees] [PATCH v2] net: fix uninit value error in __sys_sendmmsg Anant Thazhemadam
2020-09-13 21:39 ` David Miller
2020-09-14  8:14 ` Anant Thazhemadam

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).