Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] vhost: block speculation of translated descriptors
@ 2019-09-11 12:10 Michael S. Tsirkin
  2019-09-11 12:16 ` Michal Hocko
  2019-09-11 13:52 ` Michael S. Tsirkin
  0 siblings, 2 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2019-09-11 12:10 UTC (permalink / raw)
  To: linux-kernel; +Cc: Jason Wang, kvm, virtualization, netdev

iovec addresses coming from vhost are assumed to be
pre-validated, but in fact can be speculated to a value
out of range.

Userspace address are later validated with array_index_nospec so we can
be sure kernel info does not leak through these addresses, but vhost
must also not leak userspace info outside the allowed memory table to
guests.

Following the defence in depth principle, make sure
the address is not validated out of node range.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
---

changes from v1: fix build on 32 bit

 drivers/vhost/vhost.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 5dc174ac8cac..34ea219936e3 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2071,8 +2071,10 @@ static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
 		_iov = iov + ret;
 		size = node->size - addr + node->start;
 		_iov->iov_len = min((u64)len - s, size);
-		_iov->iov_base = (void __user *)(unsigned long)
-			(node->userspace_addr + addr - node->start);
+		_iov->iov_base = (void __user *)
+			((unsigned long)node->userspace_addr +
+			 array_index_nospec((unsigned long)(addr - node->start),
+					    node->size));
 		s += size;
 		addr += size;
 		++ret;
-- 
MST

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

* Re: [PATCH v2] vhost: block speculation of translated descriptors
  2019-09-11 12:10 [PATCH v2] vhost: block speculation of translated descriptors Michael S. Tsirkin
@ 2019-09-11 12:16 ` Michal Hocko
  2019-09-11 12:25   ` Michael S. Tsirkin
  2019-09-11 13:52 ` Michael S. Tsirkin
  1 sibling, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2019-09-11 12:16 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: linux-kernel, Jason Wang, kvm, virtualization, netdev

On Wed 11-09-19 08:10:00, Michael S. Tsirkin wrote:
> iovec addresses coming from vhost are assumed to be
> pre-validated, but in fact can be speculated to a value
> out of range.
> 
> Userspace address are later validated with array_index_nospec so we can
> be sure kernel info does not leak through these addresses, but vhost
> must also not leak userspace info outside the allowed memory table to
> guests.
> 
> Following the defence in depth principle, make sure
> the address is not validated out of node range.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> Tested-by: Jason Wang <jasowang@redhat.com>

no need to mark fo stable? Other spectre fixes tend to be backported
even when the security implications are not really clear. The risk
should be low and better to be covered in case.

> ---
> 
> changes from v1: fix build on 32 bit
> 
>  drivers/vhost/vhost.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 5dc174ac8cac..34ea219936e3 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2071,8 +2071,10 @@ static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
>  		_iov = iov + ret;
>  		size = node->size - addr + node->start;
>  		_iov->iov_len = min((u64)len - s, size);
> -		_iov->iov_base = (void __user *)(unsigned long)
> -			(node->userspace_addr + addr - node->start);
> +		_iov->iov_base = (void __user *)
> +			((unsigned long)node->userspace_addr +
> +			 array_index_nospec((unsigned long)(addr - node->start),
> +					    node->size));
>  		s += size;
>  		addr += size;
>  		++ret;
> -- 
> MST

-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH v2] vhost: block speculation of translated descriptors
  2019-09-11 12:16 ` Michal Hocko
@ 2019-09-11 12:25   ` Michael S. Tsirkin
  2019-09-11 12:33     ` Michal Hocko
  0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2019-09-11 12:25 UTC (permalink / raw)
  To: Michal Hocko; +Cc: linux-kernel, Jason Wang, kvm, virtualization, netdev

On Wed, Sep 11, 2019 at 02:16:28PM +0200, Michal Hocko wrote:
> On Wed 11-09-19 08:10:00, Michael S. Tsirkin wrote:
> > iovec addresses coming from vhost are assumed to be
> > pre-validated, but in fact can be speculated to a value
> > out of range.
> > 
> > Userspace address are later validated with array_index_nospec so we can
> > be sure kernel info does not leak through these addresses, but vhost
> > must also not leak userspace info outside the allowed memory table to
> > guests.
> > 
> > Following the defence in depth principle, make sure
> > the address is not validated out of node range.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > Tested-by: Jason Wang <jasowang@redhat.com>
> 
> no need to mark fo stable? Other spectre fixes tend to be backported
> even when the security implications are not really clear. The risk
> should be low and better to be covered in case.

This is not really a fix - more a defence in depth thing,
quite similar to e.g.  commit b3bbfb3fb5d25776b8e3f361d2eedaabb0b496cd
x86: Introduce __uaccess_begin_nospec() and uaccess_try_nospec
in scope.

That one doesn't seem to be tagged for stable. Was it queued
there in practice?

> > ---
> > 
> > changes from v1: fix build on 32 bit
> > 
> >  drivers/vhost/vhost.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 5dc174ac8cac..34ea219936e3 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2071,8 +2071,10 @@ static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
> >  		_iov = iov + ret;
> >  		size = node->size - addr + node->start;
> >  		_iov->iov_len = min((u64)len - s, size);
> > -		_iov->iov_base = (void __user *)(unsigned long)
> > -			(node->userspace_addr + addr - node->start);
> > +		_iov->iov_base = (void __user *)
> > +			((unsigned long)node->userspace_addr +
> > +			 array_index_nospec((unsigned long)(addr - node->start),
> > +					    node->size));
> >  		s += size;
> >  		addr += size;
> >  		++ret;
> > -- 
> > MST
> 
> -- 
> Michal Hocko
> SUSE Labs

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

* Re: [PATCH v2] vhost: block speculation of translated descriptors
  2019-09-11 12:25   ` Michael S. Tsirkin
@ 2019-09-11 12:33     ` Michal Hocko
  2019-09-11 13:03       ` Michael S. Tsirkin
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2019-09-11 12:33 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: linux-kernel, Jason Wang, kvm, virtualization, netdev

On Wed 11-09-19 08:25:03, Michael S. Tsirkin wrote:
> On Wed, Sep 11, 2019 at 02:16:28PM +0200, Michal Hocko wrote:
> > On Wed 11-09-19 08:10:00, Michael S. Tsirkin wrote:
> > > iovec addresses coming from vhost are assumed to be
> > > pre-validated, but in fact can be speculated to a value
> > > out of range.
> > > 
> > > Userspace address are later validated with array_index_nospec so we can
> > > be sure kernel info does not leak through these addresses, but vhost
> > > must also not leak userspace info outside the allowed memory table to
> > > guests.
> > > 
> > > Following the defence in depth principle, make sure
> > > the address is not validated out of node range.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > Tested-by: Jason Wang <jasowang@redhat.com>
> > 
> > no need to mark fo stable? Other spectre fixes tend to be backported
> > even when the security implications are not really clear. The risk
> > should be low and better to be covered in case.
> 
> This is not really a fix - more a defence in depth thing,
> quite similar to e.g.  commit b3bbfb3fb5d25776b8e3f361d2eedaabb0b496cd
> x86: Introduce __uaccess_begin_nospec() and uaccess_try_nospec
> in scope.
>
> That one doesn't seem to be tagged for stable. Was it queued
> there in practice?

not marked for stable but it went in. At least to 4.4.

-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH v2] vhost: block speculation of translated descriptors
  2019-09-11 12:33     ` Michal Hocko
@ 2019-09-11 13:03       ` Michael S. Tsirkin
  2019-09-11 13:12         ` Michal Hocko
  0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2019-09-11 13:03 UTC (permalink / raw)
  To: Michal Hocko; +Cc: linux-kernel, Jason Wang, kvm, virtualization, netdev

On Wed, Sep 11, 2019 at 02:33:16PM +0200, Michal Hocko wrote:
> On Wed 11-09-19 08:25:03, Michael S. Tsirkin wrote:
> > On Wed, Sep 11, 2019 at 02:16:28PM +0200, Michal Hocko wrote:
> > > On Wed 11-09-19 08:10:00, Michael S. Tsirkin wrote:
> > > > iovec addresses coming from vhost are assumed to be
> > > > pre-validated, but in fact can be speculated to a value
> > > > out of range.
> > > > 
> > > > Userspace address are later validated with array_index_nospec so we can
> > > > be sure kernel info does not leak through these addresses, but vhost
> > > > must also not leak userspace info outside the allowed memory table to
> > > > guests.
> > > > 
> > > > Following the defence in depth principle, make sure
> > > > the address is not validated out of node range.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > Tested-by: Jason Wang <jasowang@redhat.com>
> > > 
> > > no need to mark fo stable? Other spectre fixes tend to be backported
> > > even when the security implications are not really clear. The risk
> > > should be low and better to be covered in case.
> > 
> > This is not really a fix - more a defence in depth thing,
> > quite similar to e.g.  commit b3bbfb3fb5d25776b8e3f361d2eedaabb0b496cd
> > x86: Introduce __uaccess_begin_nospec() and uaccess_try_nospec
> > in scope.
> >
> > That one doesn't seem to be tagged for stable. Was it queued
> > there in practice?
> 
> not marked for stable but it went in. At least to 4.4.

So I guess the answer is I don't know. If you feel it's
justified, then sure, feel free to forward.

-- 
MST

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

* Re: [PATCH v2] vhost: block speculation of translated descriptors
  2019-09-11 13:03       ` Michael S. Tsirkin
@ 2019-09-11 13:12         ` Michal Hocko
  2019-09-11 13:51           ` Michael S. Tsirkin
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2019-09-11 13:12 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: linux-kernel, Jason Wang, kvm, virtualization, netdev

On Wed 11-09-19 09:03:10, Michael S. Tsirkin wrote:
> On Wed, Sep 11, 2019 at 02:33:16PM +0200, Michal Hocko wrote:
> > On Wed 11-09-19 08:25:03, Michael S. Tsirkin wrote:
> > > On Wed, Sep 11, 2019 at 02:16:28PM +0200, Michal Hocko wrote:
> > > > On Wed 11-09-19 08:10:00, Michael S. Tsirkin wrote:
> > > > > iovec addresses coming from vhost are assumed to be
> > > > > pre-validated, but in fact can be speculated to a value
> > > > > out of range.
> > > > > 
> > > > > Userspace address are later validated with array_index_nospec so we can
> > > > > be sure kernel info does not leak through these addresses, but vhost
> > > > > must also not leak userspace info outside the allowed memory table to
> > > > > guests.
> > > > > 
> > > > > Following the defence in depth principle, make sure
> > > > > the address is not validated out of node range.
> > > > > 
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > > Tested-by: Jason Wang <jasowang@redhat.com>
> > > > 
> > > > no need to mark fo stable? Other spectre fixes tend to be backported
> > > > even when the security implications are not really clear. The risk
> > > > should be low and better to be covered in case.
> > > 
> > > This is not really a fix - more a defence in depth thing,
> > > quite similar to e.g.  commit b3bbfb3fb5d25776b8e3f361d2eedaabb0b496cd
> > > x86: Introduce __uaccess_begin_nospec() and uaccess_try_nospec
> > > in scope.
> > >
> > > That one doesn't seem to be tagged for stable. Was it queued
> > > there in practice?
> > 
> > not marked for stable but it went in. At least to 4.4.
> 
> So I guess the answer is I don't know. If you feel it's
> justified, then sure, feel free to forward.

Well, that obviously depends on you as a maintainer but the point is
that spectre gatgets are quite hard to find. There is a smack check
AFAIK but that generates quite some false possitives and it is PITA to
crawl through those. If you want an interesting (I am not saying
vulnerable on purpose) gatget then it would be great to mark it for
stable so all stable consumers (disclaimer: I am not one of those) and
add that really great feeling of safety ;)

So take this as my 2c
-- 
Michal Hocko
SUSE Labs

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

* Re: [PATCH v2] vhost: block speculation of translated descriptors
  2019-09-11 13:12         ` Michal Hocko
@ 2019-09-11 13:51           ` Michael S. Tsirkin
  0 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2019-09-11 13:51 UTC (permalink / raw)
  To: Michal Hocko; +Cc: linux-kernel, Jason Wang, kvm, virtualization, netdev

On Wed, Sep 11, 2019 at 03:12:35PM +0200, Michal Hocko wrote:
> On Wed 11-09-19 09:03:10, Michael S. Tsirkin wrote:
> > On Wed, Sep 11, 2019 at 02:33:16PM +0200, Michal Hocko wrote:
> > > On Wed 11-09-19 08:25:03, Michael S. Tsirkin wrote:
> > > > On Wed, Sep 11, 2019 at 02:16:28PM +0200, Michal Hocko wrote:
> > > > > On Wed 11-09-19 08:10:00, Michael S. Tsirkin wrote:
> > > > > > iovec addresses coming from vhost are assumed to be
> > > > > > pre-validated, but in fact can be speculated to a value
> > > > > > out of range.
> > > > > > 
> > > > > > Userspace address are later validated with array_index_nospec so we can
> > > > > > be sure kernel info does not leak through these addresses, but vhost
> > > > > > must also not leak userspace info outside the allowed memory table to
> > > > > > guests.
> > > > > > 
> > > > > > Following the defence in depth principle, make sure
> > > > > > the address is not validated out of node range.
> > > > > > 
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > > > Tested-by: Jason Wang <jasowang@redhat.com>
> > > > > 
> > > > > no need to mark fo stable? Other spectre fixes tend to be backported
> > > > > even when the security implications are not really clear. The risk
> > > > > should be low and better to be covered in case.
> > > > 
> > > > This is not really a fix - more a defence in depth thing,
> > > > quite similar to e.g.  commit b3bbfb3fb5d25776b8e3f361d2eedaabb0b496cd
> > > > x86: Introduce __uaccess_begin_nospec() and uaccess_try_nospec
> > > > in scope.
> > > >
> > > > That one doesn't seem to be tagged for stable. Was it queued
> > > > there in practice?
> > > 
> > > not marked for stable but it went in. At least to 4.4.
> > 
> > So I guess the answer is I don't know. If you feel it's
> > justified, then sure, feel free to forward.
> 
> Well, that obviously depends on you as a maintainer but the point is
> that spectre gatgets are quite hard to find. There is a smack check
> AFAIK but that generates quite some false possitives and it is PITA to
> crawl through those. If you want an interesting (I am not saying
> vulnerable on purpose) gatget then it would be great to mark it for
> stable so all stable consumers (disclaimer: I am not one of those) and
> add that really great feeling of safety ;)
> 
> So take this as my 2c

OK it seems security@kernel.org is the way to handle these things.
I'll try that.

> -- 
> Michal Hocko
> SUSE Labs

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

* Re: [PATCH v2] vhost: block speculation of translated descriptors
  2019-09-11 12:10 [PATCH v2] vhost: block speculation of translated descriptors Michael S. Tsirkin
  2019-09-11 12:16 ` Michal Hocko
@ 2019-09-11 13:52 ` Michael S. Tsirkin
  2019-09-11 16:25   ` Will Deacon
  1 sibling, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2019-09-11 13:52 UTC (permalink / raw)
  To: linux-kernel; +Cc: Jason Wang, kvm, virtualization, netdev, security

On Wed, Sep 11, 2019 at 08:10:00AM -0400, Michael S. Tsirkin wrote:
> iovec addresses coming from vhost are assumed to be
> pre-validated, but in fact can be speculated to a value
> out of range.
> 
> Userspace address are later validated with array_index_nospec so we can
> be sure kernel info does not leak through these addresses, but vhost
> must also not leak userspace info outside the allowed memory table to
> guests.
> 
> Following the defence in depth principle, make sure
> the address is not validated out of node range.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> Tested-by: Jason Wang <jasowang@redhat.com>
> ---

Cc: security@kernel.org

Pls advise on whether you'd like me to merge this directly,
Cc stable, or handle it in some other way.

> changes from v1: fix build on 32 bit
> 
>  drivers/vhost/vhost.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 5dc174ac8cac..34ea219936e3 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2071,8 +2071,10 @@ static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
>  		_iov = iov + ret;
>  		size = node->size - addr + node->start;
>  		_iov->iov_len = min((u64)len - s, size);
> -		_iov->iov_base = (void __user *)(unsigned long)
> -			(node->userspace_addr + addr - node->start);
> +		_iov->iov_base = (void __user *)
> +			((unsigned long)node->userspace_addr +
> +			 array_index_nospec((unsigned long)(addr - node->start),
> +					    node->size));
>  		s += size;
>  		addr += size;
>  		++ret;
> -- 
> MST

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

* Re: [PATCH v2] vhost: block speculation of translated descriptors
  2019-09-11 13:52 ` Michael S. Tsirkin
@ 2019-09-11 16:25   ` Will Deacon
  0 siblings, 0 replies; 9+ messages in thread
From: Will Deacon @ 2019-09-11 16:25 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Jason Wang, kvm, virtualization, netdev, security

On Wed, Sep 11, 2019 at 09:52:25AM -0400, Michael S. Tsirkin wrote:
> On Wed, Sep 11, 2019 at 08:10:00AM -0400, Michael S. Tsirkin wrote:
> > iovec addresses coming from vhost are assumed to be
> > pre-validated, but in fact can be speculated to a value
> > out of range.
> > 
> > Userspace address are later validated with array_index_nospec so we can
> > be sure kernel info does not leak through these addresses, but vhost
> > must also not leak userspace info outside the allowed memory table to
> > guests.
> > 
> > Following the defence in depth principle, make sure
> > the address is not validated out of node range.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > Tested-by: Jason Wang <jasowang@redhat.com>
> > ---
> 
> Cc: security@kernel.org
> 
> Pls advise on whether you'd like me to merge this directly,
> Cc stable, or handle it in some other way.

I think you're fine taking it directly, with a cc stable and a Fixes: tag.

Cheers,

Will

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

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11 12:10 [PATCH v2] vhost: block speculation of translated descriptors Michael S. Tsirkin
2019-09-11 12:16 ` Michal Hocko
2019-09-11 12:25   ` Michael S. Tsirkin
2019-09-11 12:33     ` Michal Hocko
2019-09-11 13:03       ` Michael S. Tsirkin
2019-09-11 13:12         ` Michal Hocko
2019-09-11 13:51           ` Michael S. Tsirkin
2019-09-11 13:52 ` Michael S. Tsirkin
2019-09-11 16:25   ` Will Deacon

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox