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:   Wed, 6 Oct 2021 18:18:05 +0200
From:   Andrea Parri <parri.andrea@gmail.com>
To:     Michael Kelley <mikelley@microsoft.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>,
        "James E . J . Bottomley" <jejb@linux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        Dexuan Cui <decui@microsoft.com>
Subject: Re: [PATCH v2] scsi: storvsc: Fix validation for unsolicited
 incoming packets
Message-ID: <20211006161805.GA24396@anparri>
References: <20211006132026.4089-1-parri.andrea@gmail.com>
 <MWHPR21MB1593050119EACC0E49748209D7B09@MWHPR21MB1593.namprd21.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <MWHPR21MB1593050119EACC0E49748209D7B09@MWHPR21MB1593.namprd21.prod.outlook.com>
Precedence: bulk

> > @@ -1302,13 +1306,25 @@ static void storvsc_on_channel_callback(void *context)
> >  			if (rqst_id == 0) {
> >  				/*
> >  				 * storvsc_on_receive() looks at the vstor_packet in the message
> > -				 * from the ring buffer.  If the operation in the vstor_packet is
> > -				 * COMPLETE_IO, then we call storvsc_on_io_completion(), and
> > -				 * dereference the guest memory address.  Make sure we don't call
> > -				 * storvsc_on_io_completion() with a guest memory address that is
> > -				 * zero if Hyper-V were to construct and send such a bogus packet.
> > +				 * from the ring buffer.
> > +				 *
> > +				 * - If the operation in the vstor_packet is COMPLETE_IO, then
> > +				 *   we call storvsc_on_io_completion(), and dereference the
> > +				 *   guest memory address.  Make sure we don't call
> > +				 *   storvsc_on_io_completion() with a guest memory address
> > +				 *   that is zero if Hyper-V were to construct and send such
> > +				 *   a bogus packet.
> > +				 *
> > +				 * - If the operation in the vstor_packet is FCHBA_DATA, then
> > +				 *   we call cache_wwn(), and access the data payload area of
> > +				 *   the packet (wwn_packet); however, there is no guarantee
> > +				 *   that the packet is big enough to contain such area.
> > +				 *   Future-proof the code by rejecting such a bogus packet.
> 
> The comments look good to me.
> 
> > +				 *
> > +				 * XXX.  Filter out all "invalid" operations.
> 
> Is this a leftover comment line that should be deleted?  I'm not sure about the "XXX".

That was/is intended as a "TODO".  What I think we are missing here is a
specification/authority stating "allowed vstor_operation for unsolicited
messages are: ENUMERATE_BUS, REMOVE_DEVICE, etc.".  If we wanted to make
this code even more "future-proof"/"robust", we would reject all packets
whose "operation" doesn't match that list (independently from the actual
form/implementation of storvsc_on_receive()...).  We are not quite there
tough AFAICT.


> >  				 */
> > -				if (packet->operation == VSTOR_OPERATION_COMPLETE_IO) {
> > +				if (packet->operation == VSTOR_OPERATION_COMPLETE_IO ||
> > +				    packet->operation == VSTOR_OPERATION_FCHBA_DATA) {
> >  					dev_err(&device->device, "Invalid packet with ID of 0\n");
> >  					continue;
> >  				}
> > --
> > 2.25.1
> 
> Other than the seemingly spurious comment line,
> 
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>

I wanted to make sure that we're on the same page: I could either expand
or just remove that comment line; no strong opinion.  Please let me know
what is your/reviewers' preference.

Thanks,
  Andrea