From: Andrea Parri (Microsoft) <parri.andrea@xxxxxxxxx> Sent: Monday, November 9, 2020 2:07 AM
>
> From: Andres Beltran <lkmlabelt@xxxxxxxxx>
>
> Pointers to ring-buffer packets sent by Hyper-V are used 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, return a copy of the incoming VMBus packet
> after validating its length and offset fields in hv_pkt_iter_first().
> In this way, the packet can no longer be modified by the host.
>
[snip]
> @@ -419,17 +446,52 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info *rbi)
> struct vmpacket_descriptor *hv_pkt_iter_first(struct vmbus_channel *channel)
> {
> struct hv_ring_buffer_info *rbi = &channel->inbound;
> - struct vmpacket_descriptor *desc;
> + struct vmpacket_descriptor *desc, *desc_copy;
> + u32 bytes_avail, pkt_len, pkt_offset;
>
> - hv_debug_delay_test(channel, MESSAGE_DELAY);
> - if (hv_pkt_iter_avail(rbi) < sizeof(struct vmpacket_descriptor))
> + desc = hv_pkt_iter_first_raw(channel);
> + if (!desc)
> return NULL;
>
> - desc = hv_get_ring_buffer(rbi) + rbi->priv_read_index;
> - if (desc)
> - prefetch((char *)desc + (desc->len8 << 3));
> + bytes_avail = hv_pkt_iter_avail(rbi);
> +
> + /*
> + * Ensure the compiler does not use references to incoming Hyper-V values (which
> + * could change at any moment) when reading local variables later in the code
> + */
> + pkt_len = READ_ONCE(desc->len8) << 3;
> + pkt_offset = READ_ONCE(desc->offset8) << 3;
> +
> + /*
> + * If pkt_len is invalid, set it to the smaller of hv_pkt_iter_avail() and
> + * rbi->pkt_buffer_size
> + */
> + if (rbi->pkt_buffer_size < bytes_avail)
> + bytes_avail = rbi->pkt_buffer_size;
I think the above could be combined with the earlier call to hv_pkt_iter_avail(),
and more logically expressed as:
bytes_avail = min(rbi->pkt_buffer_size, hv_pkt_iter_avail(rbi));
This is a minor nit. Everything else in this patch looks good to me.
Michael
> +
> + if (pkt_len < sizeof(struct vmpacket_descriptor) || pkt_len > bytes_avail)
> + pkt_len = bytes_avail;
> +
> + /*
> + * If pkt_offset is invalid, arbitrarily set it to
> + * the size of vmpacket_descriptor
> + */
> + if (pkt_offset < sizeof(struct vmpacket_descriptor) || pkt_offset > pkt_len)
> + pkt_offset = sizeof(struct vmpacket_descriptor);
> +
> + /* Copy the Hyper-V packet out of the ring buffer */
> + desc_copy = (struct vmpacket_descriptor *)rbi->pkt_buffer;
> + memcpy(desc_copy, desc, pkt_len);
> +
> + /*
> + * Hyper-V could still change len8 and offset8 after the earlier read.
> + * Ensure that desc_copy has legal values for len8 and offset8 that
> + * are consistent with the copy we just made
> + */
> + desc_copy->len8 = pkt_len >> 3;
> + desc_copy->offset8 = pkt_offset >> 3;
>
> - return desc;
> + return desc_copy;
> }
> EXPORT_SYMBOL_GPL(hv_pkt_iter_first);
>
