Changes since RFC[1]:
- Massage changelogs, fix typo
- Drop "hv_sock: Initialize send_buf in hvs_stream_enqueue()"
- Remove style/newline change
- Remove/"inline" hv_pkt_iter_first_raw()
Applies to v5.18-rc3.
Thanks,
Andrea
[1] https://lkml.kernel.org/r/[email protected]
Andrea Parri (Microsoft) (5):
hv_sock: Check hv_pkt_iter_first_raw()'s return value
hv_sock: Copy packets sent by Hyper-V out of the ring buffer
hv_sock: Add validation for untrusted Hyper-V values
Drivers: hv: vmbus: Accept hv_sock offers in isolated guests
Drivers: hv: vmbus: Refactor the ring-buffer iterator functions
drivers/hv/channel_mgmt.c | 8 ++++--
drivers/hv/ring_buffer.c | 32 ++++++---------------
include/linux/hyperv.h | 48 ++++++++++----------------------
net/vmw_vsock/hyperv_transport.c | 22 ++++++++++++---
4 files changed, 48 insertions(+), 62 deletions(-)
--
2.25.1
With no users of hv_pkt_iter_next_raw() and no "external" users of
hv_pkt_iter_first_raw(), the iterator functions can be refactored
and simplified to remove some indirection/code.
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
---
drivers/hv/ring_buffer.c | 32 +++++++++-----------------------
include/linux/hyperv.h | 35 ++++-------------------------------
2 files changed, 13 insertions(+), 54 deletions(-)
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 3d215d9dec433..fa98b3a91206a 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -421,7 +421,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
memcpy(buffer, (const char *)desc + offset, packetlen);
/* Advance ring index to next packet descriptor */
- __hv_pkt_iter_next(channel, desc, true);
+ __hv_pkt_iter_next(channel, desc);
/* Notify host of update */
hv_pkt_iter_close(channel);
@@ -456,22 +456,6 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info *rbi)
return (rbi->ring_datasize - priv_read_loc) + write_loc;
}
-/*
- * Get first vmbus packet without copying it out of the ring buffer
- */
-struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel)
-{
- struct hv_ring_buffer_info *rbi = &channel->inbound;
-
- hv_debug_delay_test(channel, MESSAGE_DELAY);
-
- if (hv_pkt_iter_avail(rbi) < sizeof(struct vmpacket_descriptor))
- return NULL;
-
- return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index);
-}
-EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
-
/*
* Get first vmbus packet from ring buffer after read_index
*
@@ -483,11 +467,14 @@ struct vmpacket_descriptor *hv_pkt_iter_first(struct vmbus_channel *channel)
struct vmpacket_descriptor *desc, *desc_copy;
u32 bytes_avail, pkt_len, pkt_offset;
- desc = hv_pkt_iter_first_raw(channel);
- if (!desc)
+ hv_debug_delay_test(channel, MESSAGE_DELAY);
+
+ bytes_avail = hv_pkt_iter_avail(rbi);
+ if (bytes_avail < sizeof(struct vmpacket_descriptor))
return NULL;
+ bytes_avail = min(rbi->pkt_buffer_size, bytes_avail);
- bytes_avail = min(rbi->pkt_buffer_size, hv_pkt_iter_avail(rbi));
+ desc = (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index);
/*
* Ensure the compiler does not use references to incoming Hyper-V values (which
@@ -534,8 +521,7 @@ EXPORT_SYMBOL_GPL(hv_pkt_iter_first);
*/
struct vmpacket_descriptor *
__hv_pkt_iter_next(struct vmbus_channel *channel,
- const struct vmpacket_descriptor *desc,
- bool copy)
+ const struct vmpacket_descriptor *desc)
{
struct hv_ring_buffer_info *rbi = &channel->inbound;
u32 packetlen = desc->len8 << 3;
@@ -548,7 +534,7 @@ __hv_pkt_iter_next(struct vmbus_channel *channel,
rbi->priv_read_index -= dsize;
/* more data? */
- return copy ? hv_pkt_iter_first(channel) : hv_pkt_iter_first_raw(channel);
+ return hv_pkt_iter_first(channel);
}
EXPORT_SYMBOL_GPL(__hv_pkt_iter_next);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 1112c5cf894e6..370adc9971d3e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1673,55 +1673,28 @@ static inline u32 hv_pkt_len(const struct vmpacket_descriptor *desc)
return desc->len8 << 3;
}
-struct vmpacket_descriptor *
-hv_pkt_iter_first_raw(struct vmbus_channel *channel);
-
struct vmpacket_descriptor *
hv_pkt_iter_first(struct vmbus_channel *channel);
struct vmpacket_descriptor *
__hv_pkt_iter_next(struct vmbus_channel *channel,
- const struct vmpacket_descriptor *pkt,
- bool copy);
+ const struct vmpacket_descriptor *pkt);
void hv_pkt_iter_close(struct vmbus_channel *channel);
static inline struct vmpacket_descriptor *
-hv_pkt_iter_next_pkt(struct vmbus_channel *channel,
- const struct vmpacket_descriptor *pkt,
- bool copy)
+hv_pkt_iter_next(struct vmbus_channel *channel,
+ const struct vmpacket_descriptor *pkt)
{
struct vmpacket_descriptor *nxt;
- nxt = __hv_pkt_iter_next(channel, pkt, copy);
+ nxt = __hv_pkt_iter_next(channel, pkt);
if (!nxt)
hv_pkt_iter_close(channel);
return nxt;
}
-/*
- * Get next packet descriptor without copying it out of the ring buffer
- * If at end of list, return NULL and update host.
- */
-static inline struct vmpacket_descriptor *
-hv_pkt_iter_next_raw(struct vmbus_channel *channel,
- const struct vmpacket_descriptor *pkt)
-{
- return hv_pkt_iter_next_pkt(channel, pkt, false);
-}
-
-/*
- * Get next packet descriptor from iterator
- * If at end of list, return NULL and update host.
- */
-static inline struct vmpacket_descriptor *
-hv_pkt_iter_next(struct vmbus_channel *channel,
- const struct vmpacket_descriptor *pkt)
-{
- return hv_pkt_iter_next_pkt(channel, pkt, true);
-}
-
#define foreach_vmbus_pkt(pkt, channel) \
for (pkt = hv_pkt_iter_first(channel); pkt; \
pkt = hv_pkt_iter_next(channel, pkt))
--
2.25.1
From: Andrea Parri (Microsoft) <[email protected]> Sent: Wednesday, April 20, 2022 1:07 PM
>
> With no users of hv_pkt_iter_next_raw() and no "external" users of
> hv_pkt_iter_first_raw(), the iterator functions can be refactored
> and simplified to remove some indirection/code.
>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> ---
> drivers/hv/ring_buffer.c | 32 +++++++++-----------------------
> include/linux/hyperv.h | 35 ++++-------------------------------
> 2 files changed, 13 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 3d215d9dec433..fa98b3a91206a 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -421,7 +421,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
> memcpy(buffer, (const char *)desc + offset, packetlen);
>
> /* Advance ring index to next packet descriptor */
> - __hv_pkt_iter_next(channel, desc, true);
> + __hv_pkt_iter_next(channel, desc);
>
> /* Notify host of update */
> hv_pkt_iter_close(channel);
> @@ -456,22 +456,6 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info
> *rbi)
> return (rbi->ring_datasize - priv_read_loc) + write_loc;
> }
>
> -/*
> - * Get first vmbus packet without copying it out of the ring buffer
> - */
> -struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel)
> -{
> - struct hv_ring_buffer_info *rbi = &channel->inbound;
> -
> - hv_debug_delay_test(channel, MESSAGE_DELAY);
> -
> - if (hv_pkt_iter_avail(rbi) < sizeof(struct vmpacket_descriptor))
> - return NULL;
> -
> - return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> >priv_read_index);
> -}
> -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw);
> -
> /*
> * Get first vmbus packet from ring buffer after read_index
> *
> @@ -483,11 +467,14 @@ struct vmpacket_descriptor *hv_pkt_iter_first(struct
> vmbus_channel *channel)
> struct vmpacket_descriptor *desc, *desc_copy;
> u32 bytes_avail, pkt_len, pkt_offset;
>
> - desc = hv_pkt_iter_first_raw(channel);
> - if (!desc)
> + hv_debug_delay_test(channel, MESSAGE_DELAY);
> +
> + bytes_avail = hv_pkt_iter_avail(rbi);
> + if (bytes_avail < sizeof(struct vmpacket_descriptor))
> return NULL;
> + bytes_avail = min(rbi->pkt_buffer_size, bytes_avail);
>
> - bytes_avail = min(rbi->pkt_buffer_size, hv_pkt_iter_avail(rbi));
> + desc = (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi-
> >priv_read_index);
>
> /*
> * Ensure the compiler does not use references to incoming Hyper-V values
> (which
> @@ -534,8 +521,7 @@ EXPORT_SYMBOL_GPL(hv_pkt_iter_first);
> */
> struct vmpacket_descriptor *
> __hv_pkt_iter_next(struct vmbus_channel *channel,
> - const struct vmpacket_descriptor *desc,
> - bool copy)
> + const struct vmpacket_descriptor *desc)
> {
> struct hv_ring_buffer_info *rbi = &channel->inbound;
> u32 packetlen = desc->len8 << 3;
> @@ -548,7 +534,7 @@ __hv_pkt_iter_next(struct vmbus_channel *channel,
> rbi->priv_read_index -= dsize;
>
> /* more data? */
> - return copy ? hv_pkt_iter_first(channel) : hv_pkt_iter_first_raw(channel);
> + return hv_pkt_iter_first(channel);
> }
> EXPORT_SYMBOL_GPL(__hv_pkt_iter_next);
>
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 1112c5cf894e6..370adc9971d3e 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1673,55 +1673,28 @@ static inline u32 hv_pkt_len(const struct
> vmpacket_descriptor *desc)
> return desc->len8 << 3;
> }
>
> -struct vmpacket_descriptor *
> -hv_pkt_iter_first_raw(struct vmbus_channel *channel);
> -
> struct vmpacket_descriptor *
> hv_pkt_iter_first(struct vmbus_channel *channel);
>
> struct vmpacket_descriptor *
> __hv_pkt_iter_next(struct vmbus_channel *channel,
> - const struct vmpacket_descriptor *pkt,
> - bool copy);
> + const struct vmpacket_descriptor *pkt);
>
> void hv_pkt_iter_close(struct vmbus_channel *channel);
>
> static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next_pkt(struct vmbus_channel *channel,
> - const struct vmpacket_descriptor *pkt,
> - bool copy)
> +hv_pkt_iter_next(struct vmbus_channel *channel,
> + const struct vmpacket_descriptor *pkt)
> {
> struct vmpacket_descriptor *nxt;
>
> - nxt = __hv_pkt_iter_next(channel, pkt, copy);
> + nxt = __hv_pkt_iter_next(channel, pkt);
> if (!nxt)
> hv_pkt_iter_close(channel);
>
> return nxt;
> }
>
> -/*
> - * Get next packet descriptor without copying it out of the ring buffer
> - * If at end of list, return NULL and update host.
> - */
> -static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next_raw(struct vmbus_channel *channel,
> - const struct vmpacket_descriptor *pkt)
> -{
> - return hv_pkt_iter_next_pkt(channel, pkt, false);
> -}
> -
> -/*
> - * Get next packet descriptor from iterator
> - * If at end of list, return NULL and update host.
> - */
> -static inline struct vmpacket_descriptor *
> -hv_pkt_iter_next(struct vmbus_channel *channel,
> - const struct vmpacket_descriptor *pkt)
> -{
> - return hv_pkt_iter_next_pkt(channel, pkt, true);
> -}
> -
> #define foreach_vmbus_pkt(pkt, channel) \
> for (pkt = hv_pkt_iter_first(channel); pkt; \
> pkt = hv_pkt_iter_next(channel, pkt))
> --
> 2.25.1
Reviewed-by: Michael Kelley <[email protected]>
For additional robustness in the face of Hyper-V errors or malicious
behavior, validate all values that originate from packets that Hyper-V
has sent to the guest in the host-to-guest ring buffer. Ensure that
invalid values cannot cause data being copied out of the bounds of the
source buffer in hvs_stream_dequeue().
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
---
include/linux/hyperv.h | 5 +++++
net/vmw_vsock/hyperv_transport.c | 11 +++++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index fe2e0179ed51e..55478a6810b60 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1663,6 +1663,11 @@ static inline u32 hv_pkt_datalen(const struct vmpacket_descriptor *desc)
return (desc->len8 << 3) - (desc->offset8 << 3);
}
+/* Get packet length associated with descriptor */
+static inline u32 hv_pkt_len(const struct vmpacket_descriptor *desc)
+{
+ return desc->len8 << 3;
+}
struct vmpacket_descriptor *
hv_pkt_iter_first_raw(struct vmbus_channel *channel);
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 8c37d07017fc4..092cadc2c866d 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -577,12 +577,19 @@ static bool hvs_dgram_allow(u32 cid, u32 port)
static int hvs_update_recv_data(struct hvsock *hvs)
{
struct hvs_recv_buf *recv_buf;
- u32 payload_len;
+ u32 pkt_len, payload_len;
+
+ pkt_len = hv_pkt_len(hvs->recv_desc);
+
+ /* Ensure the packet is big enough to read its header */
+ if (pkt_len < HVS_HEADER_LEN)
+ return -EIO;
recv_buf = (struct hvs_recv_buf *)(hvs->recv_desc + 1);
payload_len = recv_buf->hdr.data_size;
- if (payload_len > HVS_MTU_SIZE)
+ /* Ensure the packet is big enough to read its payload */
+ if (payload_len > pkt_len - HVS_HEADER_LEN || payload_len > HVS_MTU_SIZE)
return -EIO;
if (payload_len == 0)
--
2.25.1