[PATCH] vhost: zero vhost_vsock memory on allocation

[PATCH] vhost: zero vhost_vsock memory on allocation

[Vitaly Mayatskikh]

This fixes OOPS when using under-initialized vhost_vsock object.

The code had a combo of kzalloc plus vmalloc as a fallback
initially, but it has been replaced by plain kvmalloc in
commit 6c5ab6511f71 ("mm: support __GFP_REPEAT in kvmalloc_node for >32kB")

OOPS is easy to reproduce with open/ioctl after trashing the RAM.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 drivers/vhost/vsock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index bb5fc0e..9e7cb13 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -512,7 +512,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
 	/* This struct is large and allocation could fail, fall back to vmalloc
 	 * if there is no other way.
 	 */
-	vsock = kvmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
+	vsock = kvzalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
 	if (!vsock)
 		return -ENOMEM;
 
-- 
1.8.3.1

Re: [PATCH] vhost: zero vhost_vsock memory on allocation

[Michal Hocko]

On Tue 26-03-19 20:56:14, Vitaly Mayatskikh wrote:
> This fixes OOPS when using under-initialized vhost_vsock object.
> 
> The code had a combo of kzalloc plus vmalloc as a fallback
> initially, but it has been replaced by plain kvmalloc in
> commit 6c5ab6511f71 ("mm: support __GFP_REPEAT in kvmalloc_node for >32kB")
> 
> OOPS is easy to reproduce with open/ioctl after trashing the RAM.

Sorry for the screw up, that was certainly not inteded effect of the
patch.

Fixes: 6c5ab6511f71 ("mm: support __GFP_REPEAT in kvmalloc_node for >32kB")
Cc: stable # 4.12+

> Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks for catching that.

> ---
>  drivers/vhost/vsock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index bb5fc0e..9e7cb13 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -512,7 +512,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
>  	/* This struct is large and allocation could fail, fall back to vmalloc
>  	 * if there is no other way.
>  	 */
> -	vsock = kvmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> +	vsock = kvzalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
>  	if (!vsock)
>  		return -ENOMEM;
>  
> -- 
> 1.8.3.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] vhost: zero vhost_vsock memory on allocation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

On Tue, Mar 26, 2019 at 08:56:14PM -0400, Vitaly Mayatskikh wrote:
> This fixes OOPS when using under-initialized vhost_vsock object.
> 
> The code had a combo of kzalloc plus vmalloc as a fallback
> initially, but it has been replaced by plain kvmalloc in
> commit 6c5ab6511f71 ("mm: support __GFP_REPEAT in kvmalloc_node for >32kB")
> 
> OOPS is easy to reproduce with open/ioctl after trashing the RAM.

Which field was accessed before initialization?

I ask because the situation is now unclear since code remains that
assumes vsock is *not* zero-initialized:

  vsock->guest_cid = 0; /* no CID assigned yet */

  atomic_set(&vsock->queued_replies, 0);

If we're going to zalloc, let's get rid of explicit zero
initializations.  Or let's use kvmalloc() and fix the uninitialized
access.  Mixing both is confusing.

> Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
> ---
>  drivers/vhost/vsock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index bb5fc0e..9e7cb13 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -512,7 +512,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
>  	/* This struct is large and allocation could fail, fall back to vmalloc
>  	 * if there is no other way.
>  	 */
> -	vsock = kvmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> +	vsock = kvzalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
>  	if (!vsock)
>  		return -ENOMEM;
>  
> -- 
> 1.8.3.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH] vhost: zero vhost_vsock memory on allocation

[Vitaly Mayatskih]

On Wed, Mar 27, 2019 at 12:49 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Which field was accessed before initialization?
>
> I ask because the situation is now unclear since code remains that
> assumes vsock is *not* zero-initialized:
>
>   vsock->guest_cid = 0; /* no CID assigned yet */
>
>   atomic_set(&vsock->queued_replies, 0);

It was hash.

> If we're going to zalloc, let's get rid of explicit zero
> initializations.  Or let's use kvmalloc() and fix the uninitialized
> access.  Mixing both is confusing.

I would go with zalloc, since it's easier to read and it prevents
further situations like this.
A zalloc was there originally (not in fallback though).

Re: [PATCH] vhost: zero vhost_vsock memory on allocation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

On Wed, Mar 27, 2019 at 01:08:53PM -0400, Vitaly Mayatskih wrote:
> On Wed, Mar 27, 2019 at 12:49 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > Which field was accessed before initialization?
> >
> > I ask because the situation is now unclear since code remains that
> > assumes vsock is *not* zero-initialized:
> >
> >   vsock->guest_cid = 0; /* no CID assigned yet */
> >
> >   atomic_set(&vsock->queued_replies, 0);
> 
> It was hash.

vsock->hash is only read if vsock->guest_cid has already been set and
hence vsock->hash has been initialized too.  I don't see where the
problem is.

Was your tree missing commit a72b69dc083a931422cc8a5e33841aff7d5312f2
("vhost/vsock: fix uninitialized vhost_vsock->guest_cid")?

Are you sure the crash can be reproduced on linux.git/master?

Sorry for insisting on so much information but I want to make I fully
understand the issue you encountered.

> > If we're going to zalloc, let's get rid of explicit zero
> > initializations.  Or let's use kvmalloc() and fix the uninitialized
> > access.  Mixing both is confusing.
> 
> I would go with zalloc, since it's easier to read and it prevents
> further situations like this.
> A zalloc was there originally (not in fallback though).

Sounds good.  Please send a v2 that also removes the explicit zero
initialization since it's no longer needed with zalloc.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH] vhost: zero vhost_vsock memory on allocation

[Vitaly Mayatskih]

On Thu, Mar 28, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> vsock->hash is only read if vsock->guest_cid has already been set and
> hence vsock->hash has been initialized too.  I don't see where the
> problem is.
>
> Was your tree missing commit a72b69dc083a931422cc8a5e33841aff7d5312f2
> ("vhost/vsock: fix uninitialized vhost_vsock->guest_cid")?
>
> Are you sure the crash can be reproduced on linux.git/master?

You are right: we hit it on 4.14.91 while the guest_cid fix was
backported in 4.14.93.
Thus not a real issue. Sorry for confusion, vhost is something still new to me.

> Sounds good.  Please send a v2 that also removes the explicit zero
> initialization since it's no longer needed with zalloc.

I changed my mind, all the rest is initialized explicitly, let's keep
it that way.
Will send the patch shortly.

-- 
wbr, Vitaly

[PATCH] vhost/vsock: initialize vhost_vsock->hash

[Vitaly Mayatskikh]

There's no current valid use case when uninitialized hash can be read
before being written, however let's keep every vhost_vsock field
initialized just for clarity.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 drivers/vhost/vsock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index bb5fc0e..f43f4ed 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -537,6 +537,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
 	spin_lock_init(&vsock->send_pkt_list_lock);
 	INIT_LIST_HEAD(&vsock->send_pkt_list);
 	vhost_work_init(&vsock->send_pkt_work, vhost_transport_send_pkt_work);
+	INIT_HLIST_NODE(&vsock->hash);
 	return 0;
 
 out:
-- 
1.8.3.1

Re: [PATCH] vhost/vsock: initialize vhost_vsock->hash

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On Thu, Mar 28, 2019 at 01:10:57PM -0400, Vitaly Mayatskikh wrote:
> There's no current valid use case when uninitialized hash can be read
> before being written, however let's keep every vhost_vsock field
> initialized just for clarity.
> 
> Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
> ---
>  drivers/vhost/vsock.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index bb5fc0e..f43f4ed 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -537,6 +537,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
>  	spin_lock_init(&vsock->send_pkt_list_lock);
>  	INIT_LIST_HEAD(&vsock->send_pkt_list);
>  	vhost_work_init(&vsock->send_pkt_work, vhost_transport_send_pkt_work);
> +	INIT_HLIST_NODE(&vsock->hash);
>  	return 0;
>  
>  out:
> -- 
> 1.8.3.1

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]