| Message ID | 20220420200720.434717-3-parri.andrea@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | hv_sock: Hardening changes | expand |
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 20, 2022 1:07 PM > > Pointers to VMbus packets sent by Hyper-V are used by the hv_sock driver > within the guest VM. Hyper-V can send packets with erroneous values or > modify packet fields after they are processed by the guest. To defend > against these scenarios, copy the incoming packet after validating its > length and offset fields using hv_pkt_iter_{first,next}(). In this way, > the packet can no longer be modified by the host. > > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> > --- > net/vmw_vsock/hyperv_transport.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c > index 943352530936e..8c37d07017fc4 100644 > --- a/net/vmw_vsock/hyperv_transport.c > +++ b/net/vmw_vsock/hyperv_transport.c > @@ -78,6 +78,9 @@ struct hvs_send_buf { > ALIGN((payload_len), 8) + \ > VMBUS_PKT_TRAILER_SIZE) > > +/* Upper bound on the size of a VMbus packet for hv_sock */ > +#define HVS_MAX_PKT_SIZE HVS_PKT_LEN(HVS_MTU_SIZE) > + > union hvs_service_id { > guid_t srv_id; > > @@ -378,6 +381,8 @@ static void hvs_open_connection(struct vmbus_channel *chan) > rcvbuf = ALIGN(rcvbuf, HV_HYP_PAGE_SIZE); > } > > + chan->max_pkt_size = HVS_MAX_PKT_SIZE; > + > ret = vmbus_open(chan, sndbuf, rcvbuf, NULL, 0, hvs_channel_cb, > conn_from_host ? new : sk); > if (ret != 0) { > @@ -602,7 +607,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, > struct msghdr *msg, > return -EOPNOTSUPP; > > if (need_refill) { > - hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan); > + hvs->recv_desc = hv_pkt_iter_first(hvs->chan); > if (!hvs->recv_desc) > return -ENOBUFS; > ret = hvs_update_recv_data(hvs); > @@ -618,7 +623,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, > struct msghdr *msg, > > hvs->recv_data_len -= to_read; > if (hvs->recv_data_len == 0) { > - hvs->recv_desc = hv_pkt_iter_next_raw(hvs->chan, hvs->recv_desc); > + hvs->recv_desc = hv_pkt_iter_next(hvs->chan, hvs->recv_desc); > if (hvs->recv_desc) { > ret = hvs_update_recv_data(hvs); > if (ret) > -- > 2.25.1 Reviewed-by: Michael Kelley <mikelley@microsoft.com>
On Wed, Apr 20, 2022 at 10:07:17PM +0200, Andrea Parri (Microsoft) wrote: >Pointers to VMbus packets sent by Hyper-V are used by the hv_sock driver >within the guest VM. Hyper-V can send packets with erroneous values or >modify packet fields after they are processed by the guest. To defend >against these scenarios, copy the incoming packet after validating its >length and offset fields using hv_pkt_iter_{first,next}(). In this way, >the packet can no longer be modified by the host. > >Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> >--- > net/vmw_vsock/hyperv_transport.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > >diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c >index 943352530936e..8c37d07017fc4 100644 >--- a/net/vmw_vsock/hyperv_transport.c >+++ b/net/vmw_vsock/hyperv_transport.c >@@ -78,6 +78,9 @@ struct hvs_send_buf { > ALIGN((payload_len), 8) + \ > VMBUS_PKT_TRAILER_SIZE) > >+/* Upper bound on the size of a VMbus packet for hv_sock */ >+#define HVS_MAX_PKT_SIZE HVS_PKT_LEN(HVS_MTU_SIZE) >+ > union hvs_service_id { > guid_t srv_id; > >@@ -378,6 +381,8 @@ static void hvs_open_connection(struct vmbus_channel *chan) > rcvbuf = ALIGN(rcvbuf, HV_HYP_PAGE_SIZE); > } > >+ chan->max_pkt_size = HVS_MAX_PKT_SIZE; >+ premise, I don't know HyperV channels :-( Is this change necessary to use hv_pkt_iter_first() instead of hv_pkt_iter_first_raw()? If yes, then please mention that you set this value in the commit message, otherwise maybe better to have a separate patch. Thanks, Stefano > ret = vmbus_open(chan, sndbuf, rcvbuf, NULL, 0, hvs_channel_cb, > conn_from_host ? new : sk); > if (ret != 0) { >@@ -602,7 +607,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, struct msghdr *msg, > return -EOPNOTSUPP; > > if (need_refill) { >- hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan); >+ hvs->recv_desc = hv_pkt_iter_first(hvs->chan); > if (!hvs->recv_desc) > return -ENOBUFS; > ret = hvs_update_recv_data(hvs); >@@ -618,7 +623,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, struct msghdr *msg, > > hvs->recv_data_len -= to_read; > if (hvs->recv_data_len == 0) { >- hvs->recv_desc = hv_pkt_iter_next_raw(hvs->chan, hvs->recv_desc); >+ hvs->recv_desc = hv_pkt_iter_next(hvs->chan, hvs->recv_desc); > if (hvs->recv_desc) { > ret = hvs_update_recv_data(hvs); > if (ret) >-- >2.25.1 >
> > @@ -378,6 +381,8 @@ static void hvs_open_connection(struct vmbus_channel *chan) > > rcvbuf = ALIGN(rcvbuf, HV_HYP_PAGE_SIZE); > > } > > > > + chan->max_pkt_size = HVS_MAX_PKT_SIZE; > > + > > premise, I don't know HyperV channels :-( > > Is this change necessary to use hv_pkt_iter_first() instead of > hv_pkt_iter_first_raw()? Yes, the change is required to initialize the buffer which holds the copies of the incoming packets (in hv_ringbuffer_init()). > If yes, then please mention that you set this value in the commit message, > otherwise maybe better to have a separate patch. Sure, will do. Thanks, Andrea
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c index 943352530936e..8c37d07017fc4 100644 --- a/net/vmw_vsock/hyperv_transport.c +++ b/net/vmw_vsock/hyperv_transport.c @@ -78,6 +78,9 @@ struct hvs_send_buf { ALIGN((payload_len), 8) + \ VMBUS_PKT_TRAILER_SIZE) +/* Upper bound on the size of a VMbus packet for hv_sock */ +#define HVS_MAX_PKT_SIZE HVS_PKT_LEN(HVS_MTU_SIZE) + union hvs_service_id { guid_t srv_id; @@ -378,6 +381,8 @@ static void hvs_open_connection(struct vmbus_channel *chan) rcvbuf = ALIGN(rcvbuf, HV_HYP_PAGE_SIZE); } + chan->max_pkt_size = HVS_MAX_PKT_SIZE; + ret = vmbus_open(chan, sndbuf, rcvbuf, NULL, 0, hvs_channel_cb, conn_from_host ? new : sk); if (ret != 0) { @@ -602,7 +607,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, struct msghdr *msg, return -EOPNOTSUPP; if (need_refill) { - hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan); + hvs->recv_desc = hv_pkt_iter_first(hvs->chan); if (!hvs->recv_desc) return -ENOBUFS; ret = hvs_update_recv_data(hvs); @@ -618,7 +623,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, struct msghdr *msg, hvs->recv_data_len -= to_read; if (hvs->recv_data_len == 0) { - hvs->recv_desc = hv_pkt_iter_next_raw(hvs->chan, hvs->recv_desc); + hvs->recv_desc = hv_pkt_iter_next(hvs->chan, hvs->recv_desc); if (hvs->recv_desc) { ret = hvs_update_recv_data(hvs); if (ret)
Pointers to VMbus packets sent by Hyper-V are used by the hv_sock driver within the guest VM. Hyper-V can send packets with erroneous values or modify packet fields after they are processed by the guest. To defend against these scenarios, copy the incoming packet after validating its length and offset fields using hv_pkt_iter_{first,next}(). In this way, the packet can no longer be modified by the host. Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> --- net/vmw_vsock/hyperv_transport.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)