From a8c3209998afb5c4941b49e35b513cea9050cb4a Mon Sep 17 00:00:00 2001 From: Andres Beltran Date: Tue, 8 Dec 2020 05:53:11 +0100 Subject: Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer 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. Signed-off-by: Andres Beltran Co-developed-by: Andrea Parri (Microsoft) Signed-off-by: Andrea Parri (Microsoft) Cc: "David S. Miller" Cc: Jakub Kicinski Cc: "James E.J. Bottomley" Cc: "Martin K. Petersen" Cc: netdev@vger.kernel.org Cc: linux-scsi@vger.kernel.org Reviewed-by: Michael Kelley Link: https://lore.kernel.org/r/20201208045311.10244-1-parri.andrea@gmail.com Signed-off-by: Wei Liu --- include/linux/hyperv.h | 48 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-) (limited to 'include/linux') diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 5ddb479c4d4c..fbae8406d5d4 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -181,6 +181,10 @@ struct hv_ring_buffer_info { * being freed while the ring buffer is being accessed. */ struct mutex ring_buffer_mutex; + + /* Buffer that holds a copy of an incoming host packet */ + void *pkt_buffer; + u32 pkt_buffer_size; }; @@ -787,6 +791,8 @@ struct vmbus_device { bool perf_device; }; +#define VMBUS_DEFAULT_MAX_PKT_SIZE 4096 + struct vmbus_channel { struct list_head listentry; @@ -1008,6 +1014,9 @@ struct vmbus_channel { /* request/transaction ids for VMBus */ struct vmbus_requestor requestor; u32 rqstor_size; + + /* The max size of a packet on this channel */ + u32 max_pkt_size; }; u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr); @@ -1642,32 +1651,55 @@ static inline u32 hv_pkt_datalen(const struct vmpacket_descriptor *desc) } +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); + const struct vmpacket_descriptor *pkt, + bool copy); void hv_pkt_iter_close(struct vmbus_channel *channel); -/* - * 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) +hv_pkt_iter_next_pkt(struct vmbus_channel *channel, + const struct vmpacket_descriptor *pkt, + bool copy) { struct vmpacket_descriptor *nxt; - nxt = __hv_pkt_iter_next(channel, pkt); + nxt = __hv_pkt_iter_next(channel, pkt, copy); 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)) -- cgit v1.2.3 From 06caa778d8b2fbcb4ac3878751e39d116424ba9b Mon Sep 17 00:00:00 2001 From: Andres Beltran Date: Mon, 9 Nov 2020 11:07:04 +0100 Subject: hv_utils: Add validation for untrusted Hyper-V values 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 indexing off the end of the icversion_data array in vmbus_prep_negotiate_resp(). Signed-off-by: Andres Beltran Co-developed-by: Andrea Parri (Microsoft) Signed-off-by: Andrea Parri (Microsoft) Reviewed-by: Michael Kelley Link: https://lore.kernel.org/r/20201109100704.9152-1-parri.andrea@gmail.com Signed-off-by: Wei Liu --- include/linux/hyperv.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) (limited to 'include/linux') diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index fbae8406d5d4..2ea967bc17ad 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -1480,6 +1480,7 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size); #define ICMSGTYPE_SHUTDOWN 3 #define ICMSGTYPE_TIMESYNC 4 #define ICMSGTYPE_VSS 5 +#define ICMSGTYPE_FCOPY 7 #define ICMSGHDRFLAG_TRANSACTION 1 #define ICMSGHDRFLAG_REQUEST 2 @@ -1523,6 +1524,12 @@ struct icmsg_hdr { u8 reserved[2]; } __packed; +#define IC_VERSION_NEGOTIATION_MAX_VER_COUNT 100 +#define ICMSG_HDR (sizeof(struct vmbuspipe_hdr) + sizeof(struct icmsg_hdr)) +#define ICMSG_NEGOTIATE_PKT_SIZE(icframe_vercnt, icmsg_vercnt) \ + (ICMSG_HDR + offsetof(struct icmsg_negotiate, icversion_data) + \ + (((icframe_vercnt) + (icmsg_vercnt)) * sizeof(struct ic_version))) + struct icmsg_negotiate { u16 icframe_vercnt; u16 icmsg_vercnt; @@ -1578,7 +1585,7 @@ struct hyperv_service_callback { }; #define MAX_SRV_VER 0x7ffffff -extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, u8 *buf, +extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, u8 *buf, u32 buflen, const int *fw_version, int fw_vercnt, const int *srv_version, int srv_vercnt, int *nego_fw_version, int *nego_srv_version); -- cgit v1.2.3 From e4d221b42354b2e2ddb9187a806afb651eee2cda Mon Sep 17 00:00:00 2001 From: "Andrea Parri (Microsoft)" Date: Wed, 9 Dec 2020 08:08:26 +0100 Subject: Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind() An erroneous or malicious host could send multiple rescind messages for a same channel. In vmbus_onoffer_rescind(), the guest maps the channel ID to obtain a pointer to the channel object and it eventually releases such object and associated data. The host could time rescind messages and lead to an use-after-free. Add a new flag to the channel structure to make sure that only one instance of vmbus_onoffer_rescind() can get the reference to the channel object. Reported-by: Juan Vazquez Signed-off-by: Andrea Parri (Microsoft) Reviewed-by: Michael Kelley Link: https://lore.kernel.org/r/20201209070827.29335-6-parri.andrea@gmail.com Signed-off-by: Wei Liu --- include/linux/hyperv.h | 1 + 1 file changed, 1 insertion(+) (limited to 'include/linux') diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 2ea967bc17ad..f0d48a368f13 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -809,6 +809,7 @@ struct vmbus_channel { u8 monitor_bit; bool rescind; /* got rescind msg */ + bool rescind_ref; /* got rescind msg, got channel reference */ struct completion rescind_event; u32 ringbuffer_gpadlhandle; -- cgit v1.2.3 From 21a4e356d3588806307555c149b80cec3dedb180 Mon Sep 17 00:00:00 2001 From: "Andrea Parri (Microsoft)" Date: Mon, 1 Feb 2021 15:48:12 +0100 Subject: Drivers: hv: vmbus: Restrict vmbus_devices on isolated guests Only the VSCs or ICs that have been hardened and that are critical for the successful adoption of Confidential VMs should be allowed if the guest is running isolated. This change reduces the footprint of the code that will be exercised by Confidential VMs and hence the exposure to bugs and vulnerabilities. Signed-off-by: Andrea Parri (Microsoft) Reviewed-by: Michael Kelley Link: https://lore.kernel.org/r/20210201144814.2701-3-parri.andrea@gmail.com Signed-off-by: Wei Liu --- include/linux/hyperv.h | 1 + 1 file changed, 1 insertion(+) (limited to 'include/linux') diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index f0d48a368f13..e3426f8c12db 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -789,6 +789,7 @@ struct vmbus_device { u16 dev_type; guid_t guid; bool perf_device; + bool allowed_in_isolated; }; #define VMBUS_DEFAULT_MAX_PKT_SIZE 4096 -- cgit v1.2.3 From 78785010d428f7755bf51d1c08cb2566a73dc7f5 Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Mon, 1 Feb 2021 11:43:34 -0600 Subject: hv: hyperv.h: Replace one-element array with flexible-array in struct icmsg_negotiate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is a regular need in the kernel to provide a way to declare having a dynamically sized set of trailing elements in a structure. Kernel code should always use “flexible array members”[1] for these cases. The older style of one-element or zero-length arrays should no longer be used[2]. Refactor the code according to the use of a flexible-array member in struct icmsg_negotiate, instead of a one-element array. Also, this helps the ongoing efforts to enable -Warray-bounds and fix the following warnings: drivers/hv/channel_mgmt.c:315:23: warning: array subscript 1 is above array bounds of ‘struct ic_version[1]’ [-Warray-bounds] drivers/hv/channel_mgmt.c:316:23: warning: array subscript 1 is above array bounds of ‘struct ic_version[1]’ [-Warray-bounds] [1] https://en.wikipedia.org/wiki/Flexible_array_member [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays Link: https://github.com/KSPP/linux/issues/79 Link: https://github.com/KSPP/linux/issues/109 Signed-off-by: Gustavo A. R. Silva Reviewed-by: Michael Kelley Link: https://lore.kernel.org/r/20210201174334.GA171933@embeddedor Signed-off-by: Wei Liu --- include/linux/hyperv.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include/linux') diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index e3426f8c12db..9dd22af1b7f6 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -1529,14 +1529,14 @@ struct icmsg_hdr { #define IC_VERSION_NEGOTIATION_MAX_VER_COUNT 100 #define ICMSG_HDR (sizeof(struct vmbuspipe_hdr) + sizeof(struct icmsg_hdr)) #define ICMSG_NEGOTIATE_PKT_SIZE(icframe_vercnt, icmsg_vercnt) \ - (ICMSG_HDR + offsetof(struct icmsg_negotiate, icversion_data) + \ + (ICMSG_HDR + sizeof(struct icmsg_negotiate) + \ (((icframe_vercnt) + (icmsg_vercnt)) * sizeof(struct ic_version))) struct icmsg_negotiate { u16 icframe_vercnt; u16 icmsg_vercnt; u32 reserved; - struct ic_version icversion_data[1]; /* any size array */ + struct ic_version icversion_data[]; /* any size array */ } __packed; struct shutdown_msg_data { -- cgit v1.2.3 From 3019270282a175defc02c8331786c73e082cd2a8 Mon Sep 17 00:00:00 2001 From: Wei Liu Date: Mon, 15 Feb 2021 10:44:58 +0000 Subject: Revert "Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer" This reverts commit a8c3209998afb5c4941b49e35b513cea9050cb4a. It is reported that the said commit caused regression in netvsc. Reported-by: Andrea Parri (Microsoft) Signed-off-by: Wei Liu --- include/linux/hyperv.h | 48 ++++++++---------------------------------------- 1 file changed, 8 insertions(+), 40 deletions(-) (limited to 'include/linux') diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 9dd22af1b7f6..f1d74dcf0353 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -181,10 +181,6 @@ struct hv_ring_buffer_info { * being freed while the ring buffer is being accessed. */ struct mutex ring_buffer_mutex; - - /* Buffer that holds a copy of an incoming host packet */ - void *pkt_buffer; - u32 pkt_buffer_size; }; @@ -792,8 +788,6 @@ struct vmbus_device { bool allowed_in_isolated; }; -#define VMBUS_DEFAULT_MAX_PKT_SIZE 4096 - struct vmbus_channel { struct list_head listentry; @@ -1016,9 +1010,6 @@ struct vmbus_channel { /* request/transaction ids for VMBus */ struct vmbus_requestor requestor; u32 rqstor_size; - - /* The max size of a packet on this channel */ - u32 max_pkt_size; }; u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr); @@ -1660,44 +1651,15 @@ static inline u32 hv_pkt_datalen(const struct vmpacket_descriptor *desc) } -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) -{ - struct vmpacket_descriptor *nxt; - - nxt = __hv_pkt_iter_next(channel, pkt, copy); - 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. @@ -1706,7 +1668,13 @@ 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); + struct vmpacket_descriptor *nxt; + + nxt = __hv_pkt_iter_next(channel, pkt); + if (!nxt) + hv_pkt_iter_close(channel); + + return nxt; } #define foreach_vmbus_pkt(pkt, channel) \ -- cgit v1.2.3