Currently, VMbus drivers use pointers into guest memory as request IDs
for interactions with Hyper-V. To be more robust in the face of errors
or malicious behavior from a compromised Hyper-V, avoid exposing
guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
bad request ID that is then treated as the address of a guest data
structure with no validation. Instead, encapsulate these memory
addresses and provide small integers as request IDs.
The first patch creates the definitions for the data structure, provides
helper methods to generate new IDs and retrieve data, and
allocates/frees the memory needed for vmbus_requestor.
The second and third patches make use of vmbus_requestor to send request
IDs to Hyper-V in storvsc and netvsc respectively.
The series is based on 5.10-rc3. Changelog in the actual patches.
Andrea
Cc: James E.J. Bottomley <[email protected]>
Cc: Martin K. Petersen <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: [email protected]
Cc: [email protected]
Andres Beltran (3):
Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus
hardening
scsi: storvsc: Use vmbus_requestor to generate transaction IDs for
VMBus hardening
hv_netvsc: Use vmbus_requestor to generate transaction IDs for VMBus
hardening
drivers/hv/channel.c | 174 ++++++++++++++++++++++++++++--
drivers/hv/hyperv_vmbus.h | 3 +-
drivers/hv/ring_buffer.c | 29 ++++-
drivers/net/hyperv/hyperv_net.h | 13 +++
drivers/net/hyperv/netvsc.c | 22 ++--
drivers/net/hyperv/rndis_filter.c | 1 +
drivers/scsi/storvsc_drv.c | 26 ++++-
include/linux/hyperv.h | 23 ++++
8 files changed, 273 insertions(+), 18 deletions(-)
--
2.25.1
From: Andres Beltran <[email protected]>
Currently, pointers to guest memory are passed to Hyper-V as
transaction IDs in netvsc. In the face of errors or malicious
behavior in Hyper-V, netvsc should not expose or trust the transaction
IDs returned by Hyper-V to be valid guest memory addresses. Instead,
use small integers generated by vmbus_requestor as requests
(transaction) IDs.
Signed-off-by: Andres Beltran <[email protected]>
Co-developed-by: Andrea Parri (Microsoft) <[email protected]>
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
Reviewed-by: Michael Kelley <[email protected]>
Acked-by: Jakub Kicinski <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: [email protected]
---
Changes in v9:
- Fix order of variable declarations
Changes in v7:
- Move the allocation of the request ID after the data has been
copied into the ring buffer (cf. 1/3)
Changes in v2:
- Add casts to unsigned long to fix warnings on 32bit
- Use an inline function to get the requestor size
drivers/net/hyperv/hyperv_net.h | 13 +++++++++++++
drivers/net/hyperv/netvsc.c | 22 ++++++++++++++++------
drivers/net/hyperv/rndis_filter.c | 1 +
include/linux/hyperv.h | 1 +
4 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index a0f338cf14247..2a87cfa27ac02 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -847,6 +847,19 @@ struct nvsp_message {
#define NETVSC_XDP_HDRM 256
+#define NETVSC_MIN_OUT_MSG_SIZE (sizeof(struct vmpacket_descriptor) + \
+ sizeof(struct nvsp_message))
+#define NETVSC_MIN_IN_MSG_SIZE sizeof(struct vmpacket_descriptor)
+
+/* Estimated requestor size:
+ * out_ring_size/min_out_msg_size + in_ring_size/min_in_msg_size
+ */
+static inline u32 netvsc_rqstor_size(unsigned long ringbytes)
+{
+ return ringbytes / NETVSC_MIN_OUT_MSG_SIZE +
+ 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))
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 0c3de94b51787..4dbc0055aed0e 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -50,7 +50,7 @@ void netvsc_switch_datapath(struct net_device *ndev, bool vf)
vmbus_sendpacket(dev->channel, init_pkt,
sizeof(struct nvsp_message),
- (unsigned long)init_pkt,
+ VMBUS_RQST_ID_NO_RESPONSE,
VM_PKT_DATA_INBAND, 0);
}
@@ -163,7 +163,7 @@ static void netvsc_revoke_recv_buf(struct hv_device *device,
ret = vmbus_sendpacket(device->channel,
revoke_packet,
sizeof(struct nvsp_message),
- (unsigned long)revoke_packet,
+ VMBUS_RQST_ID_NO_RESPONSE,
VM_PKT_DATA_INBAND, 0);
/* If the failure is because the channel is rescinded;
* ignore the failure since we cannot send on a rescinded
@@ -213,7 +213,7 @@ static void netvsc_revoke_send_buf(struct hv_device *device,
ret = vmbus_sendpacket(device->channel,
revoke_packet,
sizeof(struct nvsp_message),
- (unsigned long)revoke_packet,
+ VMBUS_RQST_ID_NO_RESPONSE,
VM_PKT_DATA_INBAND, 0);
/* If the failure is because the channel is rescinded;
@@ -557,7 +557,7 @@ static int negotiate_nvsp_ver(struct hv_device *device,
ret = vmbus_sendpacket(device->channel, init_packet,
sizeof(struct nvsp_message),
- (unsigned long)init_packet,
+ VMBUS_RQST_ID_NO_RESPONSE,
VM_PKT_DATA_INBAND, 0);
return ret;
@@ -614,7 +614,7 @@ static int netvsc_connect_vsp(struct hv_device *device,
/* Send the init request */
ret = vmbus_sendpacket(device->channel, init_packet,
sizeof(struct nvsp_message),
- (unsigned long)init_packet,
+ VMBUS_RQST_ID_NO_RESPONSE,
VM_PKT_DATA_INBAND, 0);
if (ret != 0)
goto cleanup;
@@ -695,10 +695,19 @@ static void netvsc_send_tx_complete(struct net_device *ndev,
const struct vmpacket_descriptor *desc,
int budget)
{
- struct sk_buff *skb = (struct sk_buff *)(unsigned long)desc->trans_id;
struct net_device_context *ndev_ctx = netdev_priv(ndev);
+ struct sk_buff *skb;
u16 q_idx = 0;
int queue_sends;
+ u64 cmd_rqst;
+
+ cmd_rqst = vmbus_request_addr(&channel->requestor, (u64)desc->trans_id);
+ if (cmd_rqst == VMBUS_RQST_ERROR) {
+ netdev_err(ndev, "Incorrect transaction id\n");
+ return;
+ }
+
+ skb = (struct sk_buff *)(unsigned long)cmd_rqst;
/* Notify the layer above us */
if (likely(skb)) {
@@ -1520,6 +1529,7 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
netvsc_poll, NAPI_POLL_WEIGHT);
/* Open the channel */
+ device->channel->rqstor_size = netvsc_rqstor_size(netvsc_ring_bytes);
ret = vmbus_open(device->channel, netvsc_ring_bytes,
netvsc_ring_bytes, NULL, 0,
netvsc_channel_cb, net_device->chan_table);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index b22e47bcfeca1..6ae43319ece68 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1172,6 +1172,7 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
/* Set the channel before opening.*/
nvchan->channel = new_sc;
+ new_sc->rqstor_size = netvsc_rqstor_size(netvsc_ring_bytes);
ret = vmbus_open(new_sc, netvsc_ring_bytes,
netvsc_ring_bytes, NULL, 0,
netvsc_channel_cb, nvchan);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 5b6d5c4e37110..5ddb479c4d4cb 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -779,6 +779,7 @@ struct vmbus_requestor {
#define VMBUS_NO_RQSTOR U64_MAX
#define VMBUS_RQST_ERROR (U64_MAX - 1)
+#define VMBUS_RQST_ID_NO_RESPONSE (U64_MAX - 2)
struct vmbus_device {
u16 dev_type;
--
2.25.1
From: Andres Beltran <[email protected]>
Currently, pointers to guest memory are passed to Hyper-V as
transaction IDs in storvsc. In the face of errors or malicious
behavior in Hyper-V, storvsc should not expose or trust the transaction
IDs returned by Hyper-V to be valid guest memory addresses. Instead,
use small integers generated by vmbus_requestor as requests
(transaction) IDs.
Signed-off-by: Andres Beltran <[email protected]>
Co-developed-by: Andrea Parri (Microsoft) <[email protected]>
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
Reviewed-by: Michael Kelley <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: [email protected]
---
Changes in v7:
- Move the allocation of the request ID after the data has been
copied into the ring buffer (cf. 1/3)
Changes in v2:
- Add casts to unsigned long to fix warnings on 32bit
drivers/scsi/storvsc_drv.c | 26 +++++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 0c65fbd41035e..369a6c6266729 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -399,6 +399,7 @@ static int storvsc_timeout = 180;
static struct scsi_transport_template *fc_transport_template;
#endif
+static struct scsi_host_template scsi_driver;
static void storvsc_on_channel_callback(void *context);
#define STORVSC_MAX_LUNS_PER_TARGET 255
@@ -698,6 +699,12 @@ static void handle_sc_creation(struct vmbus_channel *new_sc)
memset(&props, 0, sizeof(struct vmstorage_channel_properties));
+ /*
+ * The size of vmbus_requestor is an upper bound on the number of requests
+ * that can be in-progress at any one time across all channels.
+ */
+ new_sc->rqstor_size = scsi_driver.can_queue;
+
ret = vmbus_open(new_sc,
storvsc_ringbuffer_size,
storvsc_ringbuffer_size,
@@ -1242,9 +1249,17 @@ static void storvsc_on_channel_callback(void *context)
foreach_vmbus_pkt(desc, channel) {
void *packet = hv_pkt_data(desc);
struct storvsc_cmd_request *request;
+ u64 cmd_rqst;
+
+ cmd_rqst = vmbus_request_addr(&channel->requestor,
+ desc->trans_id);
+ if (cmd_rqst == VMBUS_RQST_ERROR) {
+ dev_err(&device->device,
+ "Incorrect transaction id\n");
+ continue;
+ }
- request = (struct storvsc_cmd_request *)
- ((unsigned long)desc->trans_id);
+ request = (struct storvsc_cmd_request *)(unsigned long)cmd_rqst;
if (request == &stor_device->init_request ||
request == &stor_device->reset_request) {
@@ -1265,6 +1280,12 @@ static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size,
memset(&props, 0, sizeof(struct vmstorage_channel_properties));
+ /*
+ * The size of vmbus_requestor is an upper bound on the number of requests
+ * that can be in-progress at any one time across all channels.
+ */
+ device->channel->rqstor_size = scsi_driver.can_queue;
+
ret = vmbus_open(device->channel,
ring_size,
ring_size,
@@ -1572,7 +1593,6 @@ static int storvsc_host_reset_handler(struct scsi_cmnd *scmnd)
struct vstor_packet *vstor_packet;
int ret, t;
-
stor_device = get_out_stor_device(device);
if (!stor_device)
return FAILED;
--
2.25.1
On Mon, Nov 09, 2020 at 11:04:02AM +0100, Andrea Parri (Microsoft) wrote:
> From: Andres Beltran <[email protected]>
>
> Currently, pointers to guest memory are passed to Hyper-V as
> transaction IDs in netvsc. In the face of errors or malicious
> behavior in Hyper-V, netvsc should not expose or trust the transaction
> IDs returned by Hyper-V to be valid guest memory addresses. Instead,
> use small integers generated by vmbus_requestor as requests
> (transaction) IDs.
>
> Signed-off-by: Andres Beltran <[email protected]>
> Co-developed-by: Andrea Parri (Microsoft) <[email protected]>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> Reviewed-by: Michael Kelley <[email protected]>
> Acked-by: Jakub Kicinski <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: [email protected]
Reviewed-by: Wei Liu <[email protected]>
On Mon, Nov 09, 2020 at 11:04:01AM +0100, Andrea Parri (Microsoft) wrote:
> From: Andres Beltran <[email protected]>
>
> Currently, pointers to guest memory are passed to Hyper-V as
> transaction IDs in storvsc. In the face of errors or malicious
> behavior in Hyper-V, storvsc should not expose or trust the transaction
> IDs returned by Hyper-V to be valid guest memory addresses. Instead,
> use small integers generated by vmbus_requestor as requests
> (transaction) IDs.
>
> Signed-off-by: Andres Beltran <[email protected]>
> Co-developed-by: Andrea Parri (Microsoft) <[email protected]>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> Reviewed-by: Michael Kelley <[email protected]>
> Cc: "James E.J. Bottomley" <[email protected]>
> Cc: "Martin K. Petersen" <[email protected]>
> Cc: [email protected]
Reviewed-by: Wei Liu <[email protected]>
On Fri, Nov 13, 2020 at 11:33:27AM +0000, Wei Liu wrote:
> On Mon, Nov 09, 2020 at 11:04:01AM +0100, Andrea Parri (Microsoft) wrote:
> > From: Andres Beltran <[email protected]>
> >
> > Currently, pointers to guest memory are passed to Hyper-V as
> > transaction IDs in storvsc. In the face of errors or malicious
> > behavior in Hyper-V, storvsc should not expose or trust the transaction
> > IDs returned by Hyper-V to be valid guest memory addresses. Instead,
> > use small integers generated by vmbus_requestor as requests
> > (transaction) IDs.
> >
> > Signed-off-by: Andres Beltran <[email protected]>
> > Co-developed-by: Andrea Parri (Microsoft) <[email protected]>
> > Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> > Reviewed-by: Michael Kelley <[email protected]>
> > Cc: "James E.J. Bottomley" <[email protected]>
> > Cc: "Martin K. Petersen" <[email protected]>
> > Cc: [email protected]
>
> Reviewed-by: Wei Liu <[email protected]>
Martin already gave his ack back in July. I guess nothing substantial
changed so it should have been carried over?
Wei.
On Fri, Nov 13, 2020 at 06:54:24PM +0000, Wei Liu wrote:
> On Fri, Nov 13, 2020 at 11:33:27AM +0000, Wei Liu wrote:
> > On Mon, Nov 09, 2020 at 11:04:01AM +0100, Andrea Parri (Microsoft) wrote:
> > > From: Andres Beltran <[email protected]>
> > >
> > > Currently, pointers to guest memory are passed to Hyper-V as
> > > transaction IDs in storvsc. In the face of errors or malicious
> > > behavior in Hyper-V, storvsc should not expose or trust the transaction
> > > IDs returned by Hyper-V to be valid guest memory addresses. Instead,
> > > use small integers generated by vmbus_requestor as requests
> > > (transaction) IDs.
> > >
> > > Signed-off-by: Andres Beltran <[email protected]>
> > > Co-developed-by: Andrea Parri (Microsoft) <[email protected]>
> > > Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> > > Reviewed-by: Michael Kelley <[email protected]>
> > > Cc: "James E.J. Bottomley" <[email protected]>
> > > Cc: "Martin K. Petersen" <[email protected]>
> > > Cc: [email protected]
> >
> > Reviewed-by: Wei Liu <[email protected]>
>
> Martin already gave his ack back in July. I guess nothing substantial
> changed so it should have been carried over?
The only change here happened in v7 and consisted in moving the
allocation of the request IDs from the VSC code down into the core
vmbus_sendpacket()&co functions. As mentioned in v7 cover letter,
this change was applied to ensure that the allocation in question
is performed after the packet is copied into the ring buffer. On
a positive note, this change greatly reduced the diff of this and
the following (NetVSC) patches.
Andrea
On Fri, Nov 13, 2020 at 10:39:33PM +0100, Andrea Parri wrote:
> On Fri, Nov 13, 2020 at 06:54:24PM +0000, Wei Liu wrote:
> > On Fri, Nov 13, 2020 at 11:33:27AM +0000, Wei Liu wrote:
> > > On Mon, Nov 09, 2020 at 11:04:01AM +0100, Andrea Parri (Microsoft) wrote:
> > > > From: Andres Beltran <[email protected]>
> > > >
> > > > Currently, pointers to guest memory are passed to Hyper-V as
> > > > transaction IDs in storvsc. In the face of errors or malicious
> > > > behavior in Hyper-V, storvsc should not expose or trust the transaction
> > > > IDs returned by Hyper-V to be valid guest memory addresses. Instead,
> > > > use small integers generated by vmbus_requestor as requests
> > > > (transaction) IDs.
> > > >
> > > > Signed-off-by: Andres Beltran <[email protected]>
> > > > Co-developed-by: Andrea Parri (Microsoft) <[email protected]>
> > > > Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> > > > Reviewed-by: Michael Kelley <[email protected]>
> > > > Cc: "James E.J. Bottomley" <[email protected]>
> > > > Cc: "Martin K. Petersen" <[email protected]>
> > > > Cc: [email protected]
> > >
> > > Reviewed-by: Wei Liu <[email protected]>
> >
> > Martin already gave his ack back in July. I guess nothing substantial
> > changed so it should have been carried over?
>
> The only change here happened in v7 and consisted in moving the
> allocation of the request IDs from the VSC code down into the core
> vmbus_sendpacket()&co functions. As mentioned in v7 cover letter,
> this change was applied to ensure that the allocation in question
> is performed after the packet is copied into the ring buffer. On
> a positive note, this change greatly reduced the diff of this and
> the following (NetVSC) patches.
Martin and James, are you happy with this change? I would assume you are
because that means this patch to storvsc is leaner.
Please give an explicit ack if you can. Thanks.
Wei.
>
> Andrea
Wei,
> Martin and James, are you happy with this change? I would assume you
> are because that means this patch to storvsc is leaner.
Yes, that's fine.
Acked-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
On Mon, Nov 09, 2020 at 11:03:59AM +0100, Andrea Parri (Microsoft) wrote:
> Currently, VMbus drivers use pointers into guest memory as request IDs
> for interactions with Hyper-V. To be more robust in the face of errors
> or malicious behavior from a compromised Hyper-V, avoid exposing
> guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
> bad request ID that is then treated as the address of a guest data
> structure with no validation. Instead, encapsulate these memory
> addresses and provide small integers as request IDs.
>
> The first patch creates the definitions for the data structure, provides
> helper methods to generate new IDs and retrieve data, and
> allocates/frees the memory needed for vmbus_requestor.
>
> The second and third patches make use of vmbus_requestor to send request
> IDs to Hyper-V in storvsc and netvsc respectively.
>
> The series is based on 5.10-rc3. Changelog in the actual patches.
Applied to hyperv-next. Thanks.
I also corrected the email address in my reviewed-by tags while
committing -- should've use my @kernel.org address, not @xen.org.
Wei.