RE: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed

[Vitaly Kuznetsov]

Michael Kelley <mikelley@xxxxxxxxxxxxx> writes:

> From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>  Sent: Thursday, April 2, 2020 9:27 AM
>> 
>> 163 <freedomsky1986@xxxxxxx> writes:
>> 
>> > On 4/1/2020 6:36 PM, Vitaly Kuznetsov wrote:
>> >> Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
>> >> messages. Each message comes with a header (16 bytes) which specifies the
>> >> payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
>> >> look at the real message length and copies the whole slot to a temporary
>> >> buffer before passing it to message handlers. This is potentially dangerous
>> >> as hypervisor doesn't have to clean the whole slot when putting a new
>> >> message there and a message handler can get access to some data which
>> >> belongs to a previous message.
>> >>
>> >> Note, this is not currently a problem because all message handlers are
>> >> in-kernel but eventually we may e.g. get this exported to userspace.
>> >>
>> >> Note also, that this is not a performance critical path: messages (unlike
>> >> events) represent rare events so it doesn't really matter (from performance
>> >> point of view) if we copy too much.
>> >>
>> >> Fix the issue by taking into account the real message length. The temporary
>> >> buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
>> >>
>> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>> >> ---
>> >>   drivers/hv/vmbus_drv.c | 3 ++-
>> >>   1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> >> index 029378c27421..2b5572146358 100644
>> >> --- a/drivers/hv/vmbus_drv.c
>> >> +++ b/drivers/hv/vmbus_drv.c
>> >> @@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
>> >>   			return;
>> >>
>> >>   		INIT_WORK(&ctx->work, vmbus_onmessage_work);
>> >> -		memcpy(&ctx->msg, msg, sizeof(*msg));
>> >> +		memcpy(&ctx->msg, msg, sizeof(msg->header) +
>> >> +		       msg->header.payload_size);
>> >>
>> >
>> > Hi Vitaly:
>> >       I think we still need to check whether the payload_size passed from
>> > Hyper-V is valid or not here to avoid cross-border issue before doing
>> > copying.
>> 
>> Sure,
>> 
>> the header.payload_size must be 0 <= header.payload_size <= 240
>> 
>> I'll add the check.
>> 
>
> With this change,
>
> Reviewed-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
>
> FWIW, all of this VMbus code, as well as the drivers for the VMbus
> synthetic devices, make the fundamental assumption that Hyper-V
> is trustworthy and doesn't send any malformed messages.  However,
> starting this summer we will be submitting changes to "harden" all
> of the interactions with Hyper-V to no longer make that assumption.
> All relevant fields will be checked before being used so that incorrect
> memory references aren't made.  This patch is one small step in that
> direction. :-)

What about 'alternetive' Hyper-V implementations? :-)

On a more serious note, my understanding is: if your hypervisor
misbehaves it's 'game over' for you (as a guest), the only question is
how hard is it going to be to figure out what happened. In this
particular case if hypervisor send us garbase we probably want to fail
immediately and not try to handle it as a valid message.

Thank you for your review!

-- 
Vitaly