[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87a73tzwdv.fsf@vitty.brq.redhat.com>
Date: Thu, 02 Apr 2020 18:26:52 +0200
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: 163 <freedomsky1986@....com>, linux-hyperv@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Wei Liu <wei.liu@...nel.org>,
Tianyu Lan <Tianyu.Lan@...rosoft.com>,
Michael Kelley <mikelley@...rosoft.com>,
"K. Y. Srinivasan" <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
Stephen Hemminger <sthemmin@...rosoft.com>,
"Andrea Parri \(Microsoft\)" <parri.andrea@...il.com>
Subject: Re: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed
163 <freedomsky1986@....com> 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@...hat.com>
>> ---
>> 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.
--
Vitaly
Powered by blists - more mailing lists