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

[Michael Kelley]

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. :-)

Michael