linux-kernel - RE: [PATCH] scsi: storvsc: Set correct data length for sending SCSI command without payload

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:
 <SN6PR02MB415751506B4B116CFD081939D4E62@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Tue, 21 Jan 2025 04:22:16 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Long Li <longli@...rosoft.com>, "longli@...uxonhyperv.com"
	<longli@...uxonhyperv.com>, KY Srinivasan <kys@...rosoft.com>, Haiyang Zhang
	<haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>, Dexuan Cui
	<decui@...rosoft.com>, "James E.J. Bottomley"
	<James.Bottomley@...senPartnership.com>, "Martin K. Petersen"
	<martin.petersen@...cle.com>, James Bottomley <JBottomley@...n.com>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "stable@...nel.org" <stable@...nel.org>
Subject: RE: [PATCH] scsi: storvsc: Set correct data length for sending SCSI
 command without payload

From: Long Li <longli@...rosoft.com> Sent: Monday, January 20, 2025 3:21 PM
> 
> > > In StorVSC, payload->range.len is used to indicate if this SCSI
> > > command carries payload. This data is allocated as part of the private
> > > driver data by the upper layer and may get passed to lower driver uninitialized.
> >
> > I had always thought the private driver data *is* initialized to zero by the
> > upper layer. Indeed, scsi_queue_rq() calls scsi_prepare_cmd(), which zeros the
> > private driver data as long as the driver does not specify a custom function to
> > do the initialization (and storvsc does not).  So I'm curious -- what's the
> > execution path where this initialization doesn't happen?
> >
> > Michael
> 
> SCSI mid layer may send commands to lower driver without initializing private data.
> For example, scsi_send_eh_cmnd() may send TEST_UNIT_READY and REQUEST_SENSE
> to lower layer driver without initializing private data.

Right. Thanks for pointing out this path that I wasn't aware of. My
suggestion would be to add a little more detail in the commit message,
including identifying this path where the private data isn't zero'ed. Some
future developer will wonder what's going on and appreciate having
the specific reason provided.

> 
> I don't know if there are other places doing similar things outside scsi_error.c, but
> storvsc is already calling memset() on its private data:
> (in storvsc_queuecommand)
> memset(&cmd_request->vstor_packet, 0, sizeof(struct vstor_packet));
> 
> The assumption is that private data is not guaranteed to be 0.
> 

That memset() was added relatively recently (in 2020) when doing the driver
hardening for Confidential VMs. At the time, I was thinking it was needed
because the private data isn't zero'ed, but later discovered what
scsi_prepare_cmd() does. Then I was thinking the memset() is duplicative
and wasteful, but didn't ever go back and remove it.

It seems like the SCSI subsystem has a generic inconsistency here in that
scsi_prepare_cmd() *does* zero the private data. In an attempt to give the
low level driver a clean slate, that zero'ing is done when a command is first
assigned to a request. But in the error case, the command can be re-used,
or "hijacked" per the comment for scsi_send_eh_cmnd(), and the private
data does not get zero'ed again. If the low level driver isn't guaranteed a
clean slate, then the zero'ing in scsi_prepare_cmd() is arguably not needed.

But that generic inconsistency is a different problem for another day. I'm
good with your fix in storvsc.

Reviewed-by: Michael Kelley <mhklinux@...look.com>