RE: [PATCH] hv_netvsc: Add validation for untrusted Hyper-V values
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: 2020-08-02 22:26:17
Also in:
lkml, netdev
quoted hunk ↗ jump to hunk
-----Original Message----- From: Andres Beltran <redacted> Sent: Tuesday, July 28, 2020 6:53 PM To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang [off-list ref]; Stephen Hemminger [off-list ref]; wei.liu@kernel.org Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; Michael Kelley [off-list ref]; parri.andrea@gmail.com; Saruhan Karademir [off-list ref]; Andres Beltran [off-list ref]; David S . Miller [off-list ref]; Jakub Kicinski [off-list ref]; netdev@vger.kernel.org Subject: [PATCH] hv_netvsc: 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 an array, or subvert an existing validation via integer overflow. Ensure that outgoing packets do not have any leftover guest memory that has not been zeroed out. Cc: David S. Miller <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: netdev@vger.kernel.org Signed-off-by: Andres Beltran <redacted> --- drivers/net/hyperv/hyperv_net.h | 4 ++ drivers/net/hyperv/netvsc.c | 99 +++++++++++++++++++++++++++---- drivers/net/hyperv/netvsc_drv.c | 7 +++ drivers/net/hyperv/rndis_filter.c | 73 ++++++++++++++++++++--- 4 files changed, 163 insertions(+), 20 deletions(-)diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index f43b614f2345..7df5943fa46f 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h@@ -860,6 +860,10 @@ static inline u32 netvsc_rqstor_size(unsigned longringbytes) ringbytes / NETVSC_MIN_IN_MSG_SIZE; } +#define NETVSC_XFER_HEADER_SIZE(rng_cnt) \ + (offsetof(struct vmtransfer_page_packet_header, ranges) + \ + (rng_cnt) * sizeof(struct vmtransfer_page_range)) + struct multi_send_data { struct sk_buff *skb; /* skb containing the pkt */ struct hv_netvsc_packet *pkt; /* netvsc pkt pending */diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 79b907a29433..7aa5276a1f36 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c@@ -398,6 +398,15 @@ static int netvsc_init_buf(struct hv_device *device, net_device->recv_section_size = resp->sections[0].sub_alloc_size; net_device->recv_section_cnt = resp->sections[0].num_sub_allocs; + /* Ensure buffer will not overflow */ + if (net_device->recv_section_size < NETVSC_MTU_MIN ||(u64)net_device->recv_section_size * + (u64)net_device->recv_section_cnt > (u64)buf_size) { + netdev_err(ndev, "invalid recv_section_size %u\n", + net_device->recv_section_size); + ret = -EINVAL; + goto cleanup; + } + /* Setup receive completion ring. * Add 1 to the recv_section_cnt because at least one entry in a * ring buffer has to be empty.@@ -479,6 +488,12 @@ static int netvsc_init_buf(struct hv_device *device, /* Parse the response */ net_device->send_section_size = init_packet->msg. v1_msg.send_send_buf_complete.section_size; + if (net_device->send_section_size < NETVSC_MTU_MIN) { + netdev_err(ndev, "invalid send_section_size %u\n", + net_device->send_section_size); + ret = -EINVAL; + goto cleanup; + } /* Section count is simply the size divided by the section size. */ net_device->send_section_cnt = buf_size / net_device-quoted
send_section_size;@@ -770,12 +785,24 @@ static void netvsc_send_completion(structnet_device *ndev, int budget) { const struct nvsp_message *nvsp_packet = hv_pkt_data(desc); + u32 msglen = hv_pkt_datalen(desc); + + /* Ensure packet is big enough to read header fields */ + if (msglen < sizeof(struct nvsp_message_header)) { + netdev_err(ndev, "nvsp_message length too small: %u\n", msglen); + return; + } switch (nvsp_packet->hdr.msg_type) { case NVSP_MSG_TYPE_INIT_COMPLETE: case NVSP_MSG1_TYPE_SEND_RECV_BUF_COMPLETE: case NVSP_MSG1_TYPE_SEND_SEND_BUF_COMPLETE: case NVSP_MSG5_TYPE_SUBCHANNEL: + if (msglen < sizeof(struct nvsp_message)) { + netdev_err(ndev, "nvsp_msg5 length too small: %u\n", + msglen); + return; + }
struct nvsp_message includes all message types, so its length is the longest type, The messages from older host version are not necessarily reaching the sizeof(struct nvsp_message). Testing on both new and older hosts are recommended, in case I didn't find out all issues like this one. Thanks, - Haiyang