From: Long Li <longli@xxxxxxxxxxxxx> 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@xxxxxxxxxxx>
