linux-kernel - RE: [PATCH v2] scsi: storvsc: Fix validation for unsolicited incoming packets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <MWHPR21MB1593CFF82C879AA466FB4400D7B09@MWHPR21MB1593.namprd21.prod.outlook.com>
Date:   Wed, 6 Oct 2021 16:58:08 +0000
From:   Michael Kelley <mikelley@...rosoft.com>
To:     Andrea Parri <parri.andrea@...il.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>,
        "James E . J . Bottomley" <jejb@...ux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@...cle.com>,
        Dexuan Cui <decui@...rosoft.com>
Subject: RE: [PATCH v2] scsi: storvsc: Fix validation for unsolicited incoming
 packets

From: Andrea Parri <parri.andrea@...il.com> Sent: Wednesday, October 6, 2021 9:18 AM
> > > @@ -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.
> 

Hmmm.  I think maybe we *are* there. :-)   If we get a packet with rqst_id
of zero and a vstor operation other than COMPLETE_IO or FCHBA_DATA,
then storvsc_on_receive() will be called.  The vstor operation will be
checked there, and anything not listed in the switch statement is silently
ignored, which I think is good enough.  We could output a message
in the "default" leg of the switch statement, but it's kind of a shrug for me.

Michael

> 
> > >  				 */
> > > -				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@...rosoft.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