linux-kernel - Re: [PATCH 2/3] scsi: megaraid_sas: check user-provided offsets

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7652dab94febc8667c34996740be43ff75fccdc4.camel@linux.ibm.com>
Date:   Sun, 03 Jan 2021 12:12:36 -0800
From:   James Bottomley <jejb@...ux.ibm.com>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Phil Oester <kernel@...uxace.com>, Arnd Bergmann <arnd@...db.de>,
        Kashyap Desai <kashyap.desai@...adcom.com>,
        Sumit Saxena <sumit.saxena@...adcom.com>,
        Shivasharan S <shivasharan.srikanteshwara@...adcom.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Christoph Hellwig <hch@...radead.org>,
        "# 3.4.x" <stable@...r.kernel.org>,
        Anand Lodnoor <anand.lodnoor@...adcom.com>,
        Chandrakanth Patil <chandrakanth.patil@...adcom.com>,
        Hannes Reinecke <hare@...e.de>, megaraidlinux.pdl@...adcom.com,
        linux-scsi <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] scsi: megaraid_sas: check user-provided offsets

On Sun, 2021-01-03 at 19:49 +0100, Arnd Bergmann wrote:
> On Sun, Jan 3, 2021 at 6:00 PM James Bottomley <jejb@...ux.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