RE: [PATCH 5/5] Drivers: hv: check VMBus messages lengths

From: Vitaly Kuznetsov
Date: Fri Apr 03 2020 - 03:07:55 EST


Michael Kelley <mikelley@xxxxxxxxxxxxx> writes:

> From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> Sent: Wednesday, April 1, 2020 3:38 AM
>>
>> VMBus message handlers (channel_message_table) receive a pointer to
>> 'struct vmbus_channel_message_header' and cast it to a structure of their
>> choice, which is sometimes longer than the header. We, however, don't check
>> that the message is long enough so in case hypervisor screws up we'll be
>> accessing memory beyond what was allocated for temporary buffer.
>>
>> Previously, we used to always allocate and copy 256 bytes from message page
>> to temporary buffer but this is hardly better: in case the message is
>> shorter than we expect we'll be trying to consume garbage as some real
>> data and no memory guarding technique will be able to identify an issue.
>>
>> Introduce 'min_payload_len' to 'struct vmbus_channel_message_table_entry'
>> and check against it in vmbus_on_msg_dpc(). Note, we can't require the
>> exact length as new hypervisor versions may add extra fields to messages,
>> we only check that the message is not shorter than we expect.
>
> This assumes that the current structure definitions don't already
> include extra fields that were added in newer versions of Hyper-V. If they did,
> the minimum length test could fail on older versions of Hyper-V. But I
> looked through the structure definitions and don't see any indication that
> such extra fields have been added, so this should be OK.
>

Yes, my understanding as well. When we decide to extend some of these
structures for newer VMbus version we'll have a choice: keep the require
length minimal or implement a more somplex check (e.g. add a 'length
check' function pointer to vmbus_channel_message_table_entry() -- or
pass 'length' to all message handlers and handle it ther). But as we
currently have no such cases this will definitely look over-engineered
so I decided to go the easiest way.

>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>> ---
>> drivers/hv/channel_mgmt.c | 54 ++++++++++++++++++++++-----------------
>> drivers/hv/hyperv_vmbus.h | 1 +
>> drivers/hv/vmbus_drv.c | 6 +++++
>> 3 files changed, 37 insertions(+), 24 deletions(-)
>>
>
> Reviewed-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
>

Thanks!

--
Vitaly