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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Michael Kelley <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>  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@redhat.com>
>> ---
>>  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@microsoft.com>
>

Thanks!

-- 
Vitaly