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