Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 6 Dec 2020 19:05:59 +0100
From:   Andrea Parri <parri.andrea@gmail.com>
To:     Michael Kelley <mikelley@microsoft.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        Juan Vazquez <juvazq@microsoft.com>,
        Saruhan Karademir <skarade@microsoft.com>
Subject: Re: [PATCH 2/6] Drivers: hv: vmbus: Avoid double fetch of msgtype in
 vmbus_on_msg_dpc()
Message-ID: <20201206180559.GB3256@andrea>
References: <20201118143649.108465-1-parri.andrea@gmail.com>
 <20201118143649.108465-3-parri.andrea@gmail.com>
 <MW2PR2101MB10528F278B1BD5FA060D7F5AD7CF1@MW2PR2101MB1052.namprd21.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <MW2PR2101MB10528F278B1BD5FA060D7F5AD7CF1@MW2PR2101MB1052.namprd21.prod.outlook.com>
Precedence: bulk

On Sun, Dec 06, 2020 at 05:10:26PM +0000, Michael Kelley wrote:
> From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, November 18, 2020 6:37 AM
> > 
> > vmbus_on_msg_dpc() double fetches from msgtype.  The double fetch can
> > lead to an out-of-bound access when accessing the channel_message_table
> > array.  In turn, the use of the out-of-bound entry could lead to code
> > execution primitive (entry->message_handler()).  Avoid the double fetch
> > by saving the value of msgtype into a local variable.
> 
> The commit message is missing some context.  Why is a double fetch a
> problem?  The comments below in the code explain why, but the why
> should also be briefly explained in the commit message.

I'll integrate the commit message as suggested.


> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index 0a2711aa63a15..82b23baa446d7 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1057,6 +1057,7 @@ void vmbus_on_msg_dpc(unsigned long data)
> >  	struct hv_message *msg = (struct hv_message *)page_addr +
> >  				  VMBUS_MESSAGE_SINT;
> >  	struct vmbus_channel_message_header *hdr;
> > +	enum vmbus_channel_message_type msgtype;
> >  	const struct vmbus_channel_message_table_entry *entry;
> >  	struct onmessage_work_context *ctx;
> >  	u32 message_type = msg->header.message_type;
> > @@ -1072,12 +1073,19 @@ void vmbus_on_msg_dpc(unsigned long data)
> >  		/* no msg */
> >  		return;
> > 
> > +	/*
> > +	 * The hv_message object is in memory shared with the host.  The host
> > +	 * could erroneously or maliciously modify such object.  Make sure to
> > +	 * validate its fields and avoid double fetches whenever feasible.
> 
> The "when feasible" phrase sounds like not doing double fetches is optional in
> some circumstances.  But I think we always have to protect against modification
> of memory shared with the host.  So perhaps the comment should be more
> precise.

I guess I was imagining situations where "avoiding the double fetch"
could just not be the most "convenient" option and where we may want
to instead opt for a "full re-validation" of the data at stake (say,
fetches separated by some "complex" call chain?).  We're certainly
in sync with the general principle of protecting the guest against
modification of memory shared with the host/hypervisor.  ;-)  I'll
amend the comment accordingly.

  Andrea