RE: [PATCH v7 1/3] Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus hardening
From: Michael Kelley <hidden>
Date: 2020-09-07 22:01:19
Also in:
lkml
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Monday, September 7, 2020 9:19 AM
From: Andres Beltran <redacted> 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. Signed-off-by: Andres Beltran <redacted> Co-developed-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> ---
[snip]
quoted hunk ↗ jump to hunk
--- a/drivers/hv/ring_buffer.c +++ b/drivers/hv/ring_buffer.c@@ -248,7 +248,8 @@ void hv_ringbuffer_cleanup(struct hv_ring_buffer_info *ring_info) /* Write to the ring buffer. */ int hv_ringbuffer_write(struct vmbus_channel *channel, - const struct kvec *kv_list, u32 kv_count) + const struct kvec *kv_list, u32 kv_count, + u64 requestid) { int i; u32 bytes_avail_towrite;@@ -258,6 +259,8 @@ int hv_ringbuffer_write(struct vmbus_channel *channel, u64 prev_indices; unsigned long flags; struct hv_ring_buffer_info *outring_info = &channel->outbound; + struct vmpacket_descriptor *desc = kv_list[0].iov_base; + u64 rqst_id = VMBUS_NO_RQSTOR; if (channel->rescind) return -ENODEV;@@ -300,6 +303,22 @@ int hv_ringbuffer_write(struct vmbus_channel *channel, kv_list[i].iov_len); } + /* + * Allocate the request ID after the data has been copied into the + * ring buffer. Once this request ID is allocated, the completion + * path could find the data and free it. + */ + + if (desc->flags == VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED) { + rqst_id = vmbus_next_request_id(&channel->requestor, requestid); + if (rqst_id == VMBUS_RQST_ERROR) { + pr_err("No request id available\n"); + return -EAGAIN; + } + } + desc = hv_get_ring_buffer(outring_info) + old_write; + desc->trans_id = (rqst_id == VMBUS_NO_RQSTOR) ? requestid : rqst_id; +
This is a nit, but the above would be clearer to me if written like this:
flags = desc->flags;
if (flags == VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED) {
rqst_id = vmbus_next_request_id(&channel->requestor, requestid);
if (rqst_id == VMBUS_RQST_ERROR) {
pr_err("No request id available\n");
return -EAGAIN;
}
} else {
rqst_id = requestid;
}
desc = hv_get_ring_buffer(outring_info) + old_write;
desc->trans_id = rqst_id;
The value of the flags field controls what will be used as the value for the
rqst_id. Having another test to see which value will be used as the trans_id
somehow feels a bit redundant. And then rqst_id doesn't have to be initialized.
quoted hunk ↗ jump to hunk
/* Set previous packet start */ prev_indices = hv_get_ring_bufferindices(outring_info);@@ -319,8 +338,13 @@ int hv_ringbuffer_write(struct vmbus_channel *channel, hv_signal_on_write(old_write, channel); - if (channel->rescind) + if (channel->rescind) { + if (rqst_id != VMBUS_NO_RQSTOR) {
Of course, with my proposed change, the above test would also have to be for the value of the flags field, which actually makes the code a bit more consistent. Michael
+ /* Reclaim request ID to avoid leak of IDs */ + vmbus_request_addr(&channel->requestor, rqst_id); + } return -ENODEV; + } return 0; }