linux-kernel - RE: [PATCH v2] Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <MW2PR2101MB1052B7CBB14283AA066BF125D7CF1@MW2PR2101MB1052.namprd21.prod.outlook.com>
Date:   Sun, 6 Dec 2020 22:48:10 +0000
From:   Michael Kelley <mikelley@...rosoft.com>
To:     "Andrea Parri (Microsoft)" <parri.andrea@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        Andres Beltran <lkmlabelt@...il.com>,
        Saruhan Karademir <skarade@...rosoft.com>,
        Juan Vazquez <juvazq@...rosoft.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>
Subject: RE: [PATCH v2] Drivers: hv: vmbus: Copy packets sent by Hyper-V out
 of the ring buffer

From: Andrea Parri (Microsoft) <parri.andrea@...il.com> Sent: Monday, November 9, 2020 2:07 AM
> 
> From: Andres Beltran <lkmlabelt@...il.com>
> 
> 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);
>