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
