linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] vhost/vsock: accept only packets with the right dst_cid
@ 2019-12-06 14:39 Stefano Garzarella
  2019-12-07 20:00 ` David Miller
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Stefano Garzarella @ 2019-12-06 14:39 UTC (permalink / raw)
  To: virtualization
  Cc: Stefano Garzarella, Stefan Hajnoczi, Jason Wang, kvm,
	linux-kernel, netdev, Michael S. Tsirkin

When we receive a new packet from the guest, we check if the
src_cid is correct, but we forgot to check the dst_cid.

The host should accept only packets where dst_cid is
equal to the host CID.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vhost/vsock.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 50de0642dea6..c2d7d57e98cf 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -480,7 +480,9 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
 		virtio_transport_deliver_tap_pkt(pkt);
 
 		/* Only accept correctly addressed packets */
-		if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid)
+		if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid &&
+		    le64_to_cpu(pkt->hdr.dst_cid) ==
+		    vhost_transport_get_local_cid())
 			virtio_transport_recv_pkt(&vhost_transport, pkt);
 		else
 			virtio_transport_free_pkt(pkt);
-- 
2.23.0


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

* Re: [PATCH] vhost/vsock: accept only packets with the right dst_cid
  2019-12-06 14:39 [PATCH] vhost/vsock: accept only packets with the right dst_cid Stefano Garzarella
@ 2019-12-07 20:00 ` David Miller
  2019-12-10 14:05 ` Michael S. Tsirkin
  2019-12-11 16:03 ` Michael S. Tsirkin
  2 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2019-12-07 20:00 UTC (permalink / raw)
  To: sgarzare
  Cc: virtualization, stefanha, jasowang, kvm, linux-kernel, netdev, mst

From: Stefano Garzarella <sgarzare@redhat.com>
Date: Fri,  6 Dec 2019 15:39:12 +0100

> When we receive a new packet from the guest, we check if the
> src_cid is correct, but we forgot to check the dst_cid.
> 
> The host should accept only packets where dst_cid is
> equal to the host CID.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Applied.

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

* Re: [PATCH] vhost/vsock: accept only packets with the right dst_cid
  2019-12-06 14:39 [PATCH] vhost/vsock: accept only packets with the right dst_cid Stefano Garzarella
  2019-12-07 20:00 ` David Miller
@ 2019-12-10 14:05 ` Michael S. Tsirkin
  2019-12-10 14:32   ` Stefano Garzarella
  2019-12-11 16:03 ` Michael S. Tsirkin
  2 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-12-10 14:05 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: virtualization, Stefan Hajnoczi, Jason Wang, kvm, linux-kernel, netdev

On Fri, Dec 06, 2019 at 03:39:12PM +0100, Stefano Garzarella wrote:
> When we receive a new packet from the guest, we check if the
> src_cid is correct, but we forgot to check the dst_cid.
> 
> The host should accept only packets where dst_cid is
> equal to the host CID.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

what's the implication of processing incorrect dst cid?
I think mostly it's malformed guests, right?
Everyone else just passes the known host cid ...

> ---
>  drivers/vhost/vsock.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 50de0642dea6..c2d7d57e98cf 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -480,7 +480,9 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
>  		virtio_transport_deliver_tap_pkt(pkt);
>  
>  		/* Only accept correctly addressed packets */
> -		if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid)
> +		if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid &&
> +		    le64_to_cpu(pkt->hdr.dst_cid) ==
> +		    vhost_transport_get_local_cid())
>  			virtio_transport_recv_pkt(&vhost_transport, pkt);
>  		else
>  			virtio_transport_free_pkt(pkt);
> -- 
> 2.23.0


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

* Re: [PATCH] vhost/vsock: accept only packets with the right dst_cid
  2019-12-10 14:05 ` Michael S. Tsirkin
@ 2019-12-10 14:32   ` Stefano Garzarella
  0 siblings, 0 replies; 10+ messages in thread
From: Stefano Garzarella @ 2019-12-10 14:32 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtualization, Stefan Hajnoczi, Jason Wang, kvm, linux-kernel, netdev

On Tue, Dec 10, 2019 at 09:05:58AM -0500, Michael S. Tsirkin wrote:
> On Fri, Dec 06, 2019 at 03:39:12PM +0100, Stefano Garzarella wrote:
> > When we receive a new packet from the guest, we check if the
> > src_cid is correct, but we forgot to check the dst_cid.
> > 
> > The host should accept only packets where dst_cid is
> > equal to the host CID.
> > 
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> 
> what's the implication of processing incorrect dst cid?
> I think mostly it's malformed guests, right?

Exaclty, as for the src_cid.

In both cases the packet may be delivered to the wrong socket in the
host, because in the virtio_transport_recv_pkt() we are using the
src_cid and dst_cid to look for the socket where to queue the packet.

> Everyone else just passes the known host cid ...

Yes, good guests should do it, and we do it :-)

Thanks,
Stefano


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

* Re: [PATCH] vhost/vsock: accept only packets with the right dst_cid
  2019-12-06 14:39 [PATCH] vhost/vsock: accept only packets with the right dst_cid Stefano Garzarella
  2019-12-07 20:00 ` David Miller
  2019-12-10 14:05 ` Michael S. Tsirkin
@ 2019-12-11 16:03 ` Michael S. Tsirkin
  2019-12-12 12:36   ` Stefano Garzarella
  2 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-12-11 16:03 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: virtualization, Stefan Hajnoczi, Jason Wang, kvm, linux-kernel, netdev

On Fri, Dec 06, 2019 at 03:39:12PM +0100, Stefano Garzarella wrote:
> When we receive a new packet from the guest, we check if the
> src_cid is correct, but we forgot to check the dst_cid.
> 
> The host should accept only packets where dst_cid is
> equal to the host CID.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Stefano can you clarify the impact pls?
E.g. is this needed on stable? Etc.

Thanks!

> ---
>  drivers/vhost/vsock.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 50de0642dea6..c2d7d57e98cf 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -480,7 +480,9 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
>  		virtio_transport_deliver_tap_pkt(pkt);
>  
>  		/* Only accept correctly addressed packets */
> -		if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid)
> +		if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid &&
> +		    le64_to_cpu(pkt->hdr.dst_cid) ==
> +		    vhost_transport_get_local_cid())
>  			virtio_transport_recv_pkt(&vhost_transport, pkt);
>  		else
>  			virtio_transport_free_pkt(pkt);
> -- 
> 2.23.0


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

* Re: [PATCH] vhost/vsock: accept only packets with the right dst_cid
  2019-12-11 16:03 ` Michael S. Tsirkin
@ 2019-12-12 12:36   ` Stefano Garzarella
  2019-12-12 12:56     ` Michael S. Tsirkin
  0 siblings, 1 reply; 10+ messages in thread
From: Stefano Garzarella @ 2019-12-12 12:36 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtualization, Stefan Hajnoczi, Jason Wang, kvm, linux-kernel, netdev

On Wed, Dec 11, 2019 at 11:03:07AM -0500, Michael S. Tsirkin wrote:
> On Fri, Dec 06, 2019 at 03:39:12PM +0100, Stefano Garzarella wrote:
> > When we receive a new packet from the guest, we check if the
> > src_cid is correct, but we forgot to check the dst_cid.
> > 
> > The host should accept only packets where dst_cid is
> > equal to the host CID.
> > 
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> 
> Stefano can you clarify the impact pls?

Sure, I'm sorry I didn't do it earlier.

> E.g. is this needed on stable? Etc.

This is a better analysis (I hope) when there is a malformed guest
that sends a packet with a wrong dst_cid:
- before v5.4 we supported only one transport at runtime, so the sockets
  in the host can only receive packets from guests. In this case, if
  the dst_cid is wrong, maybe the only issue is that the getsockname()
  returns an inconsistent address (the cid returned is the one received
  from the guest)

- from v5.4 we support multi-transport, so the L1 VM (e.g. L0 assigned
  cid 5 to this VM) can have both Guest2Host and Host2Guest transports.
  In this case, we have these possible issues:
  - L2 (or L1) guest can use cid 0, 1, and 2 to reach L1 (or L0),
    instead we should allow only CID_HOST (2) to reach the level below.
    Note: this happens also with not malformed guest that runs Linux v5.4

  - if a malformed L2 guest sends a packet with the wrong dst_cid, for example
    instead of CID_HOST, it uses the cid assigned by L0 to L1 (5 in this
    example), this packets can wrongly queued to a socket on L1 bound to cid 5,
    that only expects connections from L0.

Maybe we really need this only on stable v5.4, but the patch is very simple
and should apply cleanly to all stable branches.

What do you think?

Thanks,
Stefano


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

* Re: [PATCH] vhost/vsock: accept only packets with the right dst_cid
  2019-12-12 12:36   ` Stefano Garzarella
@ 2019-12-12 12:56     ` Michael S. Tsirkin
  2019-12-12 13:14       ` Stefano Garzarella
  0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-12-12 12:56 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: virtualization, Stefan Hajnoczi, Jason Wang, kvm, linux-kernel, netdev

On Thu, Dec 12, 2019 at 01:36:24PM +0100, Stefano Garzarella wrote:
> On Wed, Dec 11, 2019 at 11:03:07AM -0500, Michael S. Tsirkin wrote:
> > On Fri, Dec 06, 2019 at 03:39:12PM +0100, Stefano Garzarella wrote:
> > > When we receive a new packet from the guest, we check if the
> > > src_cid is correct, but we forgot to check the dst_cid.
> > > 
> > > The host should accept only packets where dst_cid is
> > > equal to the host CID.
> > > 
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > 
> > Stefano can you clarify the impact pls?
> 
> Sure, I'm sorry I didn't do it earlier.
> 
> > E.g. is this needed on stable? Etc.
> 
> This is a better analysis (I hope) when there is a malformed guest
> that sends a packet with a wrong dst_cid:
> - before v5.4 we supported only one transport at runtime, so the sockets
>   in the host can only receive packets from guests. In this case, if
>   the dst_cid is wrong, maybe the only issue is that the getsockname()
>   returns an inconsistent address (the cid returned is the one received
>   from the guest)
> 
> - from v5.4 we support multi-transport, so the L1 VM (e.g. L0 assigned
>   cid 5 to this VM) can have both Guest2Host and Host2Guest transports.
>   In this case, we have these possible issues:
>   - L2 (or L1) guest can use cid 0, 1, and 2 to reach L1 (or L0),
>     instead we should allow only CID_HOST (2) to reach the level below.
>     Note: this happens also with not malformed guest that runs Linux v5.4
>   - if a malformed L2 guest sends a packet with the wrong dst_cid, for example
>     instead of CID_HOST, it uses the cid assigned by L0 to L1 (5 in this
>     example), this packets can wrongly queued to a socket on L1 bound to cid 5,
>     that only expects connections from L0.

Oh so a security issue?

> 
> Maybe we really need this only on stable v5.4, but the patch is very simple
> and should apply cleanly to all stable branches.
> 
> What do you think?
> 
> Thanks,
> Stefano

I'd say it's better to backport to all stable releases where it applies,
but yes it's only a security issue in 5.4.  Dave could you forward pls?

-- 
MST


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

* Re: [PATCH] vhost/vsock: accept only packets with the right dst_cid
  2019-12-12 12:56     ` Michael S. Tsirkin
@ 2019-12-12 13:14       ` Stefano Garzarella
  2019-12-12 17:18         ` Stefano Garzarella
  2019-12-12 19:01         ` David Miller
  0 siblings, 2 replies; 10+ messages in thread
From: Stefano Garzarella @ 2019-12-12 13:14 UTC (permalink / raw)
  To: Michael S. Tsirkin, davem
  Cc: virtualization, Stefan Hajnoczi, Jason Wang, kvm, linux-kernel, netdev

On Thu, Dec 12, 2019 at 07:56:26AM -0500, Michael S. Tsirkin wrote:
> On Thu, Dec 12, 2019 at 01:36:24PM +0100, Stefano Garzarella wrote:
> > On Wed, Dec 11, 2019 at 11:03:07AM -0500, Michael S. Tsirkin wrote:
> > > On Fri, Dec 06, 2019 at 03:39:12PM +0100, Stefano Garzarella wrote:
> > > > When we receive a new packet from the guest, we check if the
> > > > src_cid is correct, but we forgot to check the dst_cid.
> > > > 
> > > > The host should accept only packets where dst_cid is
> > > > equal to the host CID.
> > > > 
> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > 
> > > Stefano can you clarify the impact pls?
> > 
> > Sure, I'm sorry I didn't do it earlier.
> > 
> > > E.g. is this needed on stable? Etc.
> > 
> > This is a better analysis (I hope) when there is a malformed guest
> > that sends a packet with a wrong dst_cid:
> > - before v5.4 we supported only one transport at runtime, so the sockets
> >   in the host can only receive packets from guests. In this case, if
> >   the dst_cid is wrong, maybe the only issue is that the getsockname()
> >   returns an inconsistent address (the cid returned is the one received
> >   from the guest)
> > 
> > - from v5.4 we support multi-transport, so the L1 VM (e.g. L0 assigned
> >   cid 5 to this VM) can have both Guest2Host and Host2Guest transports.
> >   In this case, we have these possible issues:
> >   - L2 (or L1) guest can use cid 0, 1, and 2 to reach L1 (or L0),
> >     instead we should allow only CID_HOST (2) to reach the level below.
> >     Note: this happens also with not malformed guest that runs Linux v5.4
> >   - if a malformed L2 guest sends a packet with the wrong dst_cid, for example
> >     instead of CID_HOST, it uses the cid assigned by L0 to L1 (5 in this
> >     example), this packets can wrongly queued to a socket on L1 bound to cid 5,
> >     that only expects connections from L0.
> 
> Oh so a security issue?
> 

It seems so, I'll try to see if I can get a real example,
maybe I missed a few checks.

> > 
> > Maybe we really need this only on stable v5.4, but the patch is very simple
> > and should apply cleanly to all stable branches.
> > 
> > What do you think?
> > 
> > Thanks,
> > Stefano
> 
> I'd say it's better to backport to all stable releases where it applies,
> but yes it's only a security issue in 5.4.  Dave could you forward pls?

Yes, I agree with you.

@Dave let me know if I should do it.

Thanks,
Stefano


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

* Re: [PATCH] vhost/vsock: accept only packets with the right dst_cid
  2019-12-12 13:14       ` Stefano Garzarella
@ 2019-12-12 17:18         ` Stefano Garzarella
  2019-12-12 19:01         ` David Miller
  1 sibling, 0 replies; 10+ messages in thread
From: Stefano Garzarella @ 2019-12-12 17:18 UTC (permalink / raw)
  To: Michael S. Tsirkin, David S. Miller
  Cc: virtualization, Stefan Hajnoczi, Jason Wang, kvm, linux-kernel, netdev

On Thu, Dec 12, 2019 at 2:14 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> On Thu, Dec 12, 2019 at 07:56:26AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Dec 12, 2019 at 01:36:24PM +0100, Stefano Garzarella wrote:
> > > On Wed, Dec 11, 2019 at 11:03:07AM -0500, Michael S. Tsirkin wrote:
> > > > On Fri, Dec 06, 2019 at 03:39:12PM +0100, Stefano Garzarella wrote:
> > > > > When we receive a new packet from the guest, we check if the
> > > > > src_cid is correct, but we forgot to check the dst_cid.
> > > > >
> > > > > The host should accept only packets where dst_cid is
> > > > > equal to the host CID.
> > > > >
> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > >
> > > > Stefano can you clarify the impact pls?
> > >
> > > Sure, I'm sorry I didn't do it earlier.
> > >
> > > > E.g. is this needed on stable? Etc.
> > >
> > > This is a better analysis (I hope) when there is a malformed guest
> > > that sends a packet with a wrong dst_cid:
> > > - before v5.4 we supported only one transport at runtime, so the sockets
> > >   in the host can only receive packets from guests. In this case, if
> > >   the dst_cid is wrong, maybe the only issue is that the getsockname()
> > >   returns an inconsistent address (the cid returned is the one received
> > >   from the guest)
> > >
> > > - from v5.4 we support multi-transport, so the L1 VM (e.g. L0 assigned
> > >   cid 5 to this VM) can have both Guest2Host and Host2Guest transports.
> > >   In this case, we have these possible issues:
> > >   - L2 (or L1) guest can use cid 0, 1, and 2 to reach L1 (or L0),
> > >     instead we should allow only CID_HOST (2) to reach the level below.
> > >     Note: this happens also with not malformed guest that runs Linux v5.4
> > >   - if a malformed L2 guest sends a packet with the wrong dst_cid, for example
> > >     instead of CID_HOST, it uses the cid assigned by L0 to L1 (5 in this
> > >     example), this packets can wrongly queued to a socket on L1 bound to cid 5,
> > >     that only expects connections from L0.
> >
> > Oh so a security issue?
> >
>
> It seems so, I'll try to see if I can get a real example,
> maybe I missed a few checks.

I was wrong!
Multi-transport will be released with v5.5, which will contain this patch.

Linux <= v5.4 are safe, with the exception of the potential wrong
address returned by getsockname().

In addition, trying Linux <= v5.4 (both guests and host), I found that
userspace applications can use any dst_cid to reach the host.

It is not a security issue but for sure a wrong semantics.
Maybe we should still consider to backport this patch on stables to get
the right semantics.

Thanks,
Stefano


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

* Re: [PATCH] vhost/vsock: accept only packets with the right dst_cid
  2019-12-12 13:14       ` Stefano Garzarella
  2019-12-12 17:18         ` Stefano Garzarella
@ 2019-12-12 19:01         ` David Miller
  1 sibling, 0 replies; 10+ messages in thread
From: David Miller @ 2019-12-12 19:01 UTC (permalink / raw)
  To: sgarzare
  Cc: mst, virtualization, stefanha, jasowang, kvm, linux-kernel, netdev

From: Stefano Garzarella <sgarzare@redhat.com>
Date: Thu, 12 Dec 2019 14:14:53 +0100

> On Thu, Dec 12, 2019 at 07:56:26AM -0500, Michael S. Tsirkin wrote:
>> On Thu, Dec 12, 2019 at 01:36:24PM +0100, Stefano Garzarella wrote:
>> I'd say it's better to backport to all stable releases where it applies,
>> but yes it's only a security issue in 5.4.  Dave could you forward pls?
> 
> Yes, I agree with you.
> 
> @Dave let me know if I should do it.

I've queued it up for -stable.

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

end of thread, other threads:[~2019-12-12 19:02 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06 14:39 [PATCH] vhost/vsock: accept only packets with the right dst_cid Stefano Garzarella
2019-12-07 20:00 ` David Miller
2019-12-10 14:05 ` Michael S. Tsirkin
2019-12-10 14:32   ` Stefano Garzarella
2019-12-11 16:03 ` Michael S. Tsirkin
2019-12-12 12:36   ` Stefano Garzarella
2019-12-12 12:56     ` Michael S. Tsirkin
2019-12-12 13:14       ` Stefano Garzarella
2019-12-12 17:18         ` Stefano Garzarella
2019-12-12 19:01         ` David Miller

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