From: Andrea Parri (Microsoft) <[email protected]> Sent: Wednesday, November 18, 2020 6:37 AM > > vmbus_on_msg_dpc() double fetches from payload_size. The double fetch > can lead to a buffer overflow when (mem)copying the hv_message object. > Avoid the double fetch by saving the value of payload_size into a local > variable.
Similar comment here about providing some brief context in the commit message on the problem that we are guarding against by removing the double fetch. I could see combining this patch with the previous one since the motivation and pattern of the changes are exactly the same, just for two different fields. Michael > > Reported-by: Juan Vazquez <[email protected]> > Signed-off-by: Andrea Parri (Microsoft) <[email protected]> > --- > drivers/hv/vmbus_drv.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 82b23baa446d7..0e39f1d6182e9 100644 > --- a/drivers/hv/vmbus_drv.c > +++ b/drivers/hv/vmbus_drv.c > @@ -1056,6 +1056,7 @@ void vmbus_on_msg_dpc(unsigned long data) > void *page_addr = hv_cpu->synic_message_page; > struct hv_message *msg = (struct hv_message *)page_addr + > VMBUS_MESSAGE_SINT; > + __u8 payload_size = msg->header.payload_size; > struct vmbus_channel_message_header *hdr; > enum vmbus_channel_message_type msgtype; > const struct vmbus_channel_message_table_entry *entry; > @@ -1089,9 +1090,8 @@ void vmbus_on_msg_dpc(unsigned long data) > goto msg_handled; > } > > - if (msg->header.payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) { > - WARN_ONCE(1, "payload size is too large (%d)\n", > - msg->header.payload_size); > + if (payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) { > + WARN_ONCE(1, "payload size is too large (%d)\n", payload_size); > goto msg_handled; > } > > @@ -1100,21 +1100,18 @@ void vmbus_on_msg_dpc(unsigned long data) > if (!entry->message_handler) > goto msg_handled; > > - if (msg->header.payload_size < entry->min_payload_len) { > - WARN_ONCE(1, "message too short: msgtype=%d len=%d\n", > - msgtype, msg->header.payload_size); > + if (payload_size < entry->min_payload_len) { > + WARN_ONCE(1, "message too short: msgtype=%d len=%d\n", msgtype, > payload_size); > goto msg_handled; > } > > if (entry->handler_type == VMHT_BLOCKING) { > - ctx = kmalloc(sizeof(*ctx) + msg->header.payload_size, > - GFP_ATOMIC); > + ctx = kmalloc(sizeof(*ctx) + payload_size, GFP_ATOMIC); > if (ctx == NULL) > return; > > INIT_WORK(&ctx->work, vmbus_onmessage_work); > - memcpy(&ctx->msg, msg, sizeof(msg->header) + > - msg->header.payload_size); > + memcpy(&ctx->msg, msg, sizeof(msg->header) + payload_size); > > /* > * The host can generate a rescind message while we > -- > 2.25.1
