Messages in this thread | | | Subject | Re: [PATCH 2/3] scsi: megaraid_sas: check user-provided offsets | From | James Bottomley <> | Date | Sun, 03 Jan 2021 12:12:36 -0800 |
| |
On Sun, 2021-01-03 at 19:49 +0100, Arnd Bergmann wrote: > On Sun, Jan 3, 2021 at 6:00 PM James Bottomley <jejb@linux.ibm.com> > wrote: > > On Sun, 2021-01-03 at 17:26 +0100, Arnd Bergmann wrote: > > [...] > > > @@ -8209,7 +8208,7 @@ megasas_mgmt_fw_ioctl(struct > > > megasas_instance > > > *instance, > > > if (instance->consistent_mask_64bit) > > > put_unaligned_le64(sense_handle, > > > sense_ptr); > > > else > > > - put_unaligned_le32(sense_handle, > > > sense_ptr); > > > + put_unaligned_le64(sense_handle, > > > sense_ptr); > > > } > > > > This hunk can't be right. It effectively means removing the if. > > I'm just trying to restore the state before the regression introduced > in my 381d34e376e3 ("scsi: megaraid_sas: Check user-provided > offsets"). > > The old code always stored 'sizeof(long)' bytes into sense_ptr, > regardless of instance->consistent_mask_64bit, but it would truncate > the address to 32 bit if that was cleared. This was clearly bogus > and I tried to make it do something more meaningful, only storing > 8 bytes into the structure if it was configured for 64-bit DMA, > regardless of the capabilities of the kernel.
Heh, well, all this depends on how the firmware interprets the pointer, for which we don't seem to have a manual. Instinct tells me the flag MFI_FRAME_SENSE64 is what does this and that's conditioned on the same if clause 100 lines above this, so the fix your proposing would still seem to be wrong, because I think when that flag is not set, the device expects the sense pointer to be 32 bit.
> > However, the if is needed because sense_handle is a dma_addr_t > > which can be either 32 or 64 bit. What about changing the if to > > > > if (sizeof(dma_addr_t) == 8) > > > > instead? > > That would not be useful either, the device surely does not care > if the kernel supports 64-bit DMA. What we'd really need here is > someone with access to the interface specifications to see how > many bytes should be stored in the structure. I suspect always > storing 64 bits (as my patch does) is correct, and would send a > proper patch to remove the if() if Phil confirms that my test > patch fixes the regression.
Well, as I said above, I'm speculating the device does what we tell it, and whether to use 32 or 64 bits for the sense pointer definitely seems to be a flag the driver controls ... we really need someone with access to the programming manual to tell us if this speculation is accurate, though.
James
| |