Message ID | 20191206143912.153583-1-sgarzare@redhat.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | vhost/vsock: accept only packets with the right dst_cid | expand |
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.
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
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
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
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
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?
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
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
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.
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);
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(-)