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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 03 Apr 2020 09:07:47 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Michael Kelley <mikelley@...rosoft.com>,
        "linux-hyperv\@vger.kernel.org" <linux-hyperv@...r.kernel.org>
Cc:     "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        Wei Liu <wei.liu@...nel.org>,
        Tianyu Lan <Tianyu.Lan@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "Andrea Parri \(Microsoft\)" <parri.andrea@...il.com>
Subject: RE: [PATCH 5/5] Drivers: hv: check VMBus messages lengths

Michael Kelley <mikelley@...rosoft.com> writes:

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

Thanks!

-- 
Vitaly