linux-kernel - Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page

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: <alpine.DEB.2.21.2201032324230.56863@angie.orcam.me.uk>
Date:   Tue, 4 Jan 2022 13:52:47 +0000 (GMT)
From:   "Maciej W. Rozycki" <macro@...am.me.uk>
To:     "Martin K. Petersen" <martin.petersen@...cle.com>
cc:     Douglas Gilbert <dgilbert@...erlog.com>,
        Khalid Aziz <khalid@...ehiking.org>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        Christoph Hellwig <hch@....de>, Nix <nix@...eri.org.uk>,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA
 Information VPD page

Martin,

> >  So I do have all the reasons to conclude this value has indeed been 
> > arbitrarily chosen, don't I?
> 
> The SAT spec defines the contents of the first part of the page. The
> trailing 512 bytes are defined in the ATA spec.

 I think that would best be reflected in code somehow as lone `64' doesn't 
say anything really.

> > If you insist that the value of 64 stay, then please come up with a
> > suitable macro name to define along with SCSI_VPD_PG_LEN that reflects
> > the meaning of 64 in this context and I'll be happy to update 3/3
> > accordingly, but please explain why the value of 64 is any better than
> > 255 here and the inconsistency with the buffer size justified.
> 
> Can please you try the following patch?

 I have tried it and it's neutral, that is with 1/3 applied the HBA still 
works and with 1/3 removed it still breaks (2/3 and 3/3 obviously don't 
build anymore).  Unsurprisingly, as it's the call to `scsi_get_vpd_page' 
rather than `scsi_get_vpd_buf' that causes an issue here.

 I think the latter function isn't called in my setup, and it's not clear 
to me anymore after so long why I didn't poke at it.  It looks to me like 
the `retry_pg' code there can be gone now with your update in place as it 
duplicates buffer sizing, and with that included it'll be a nice clean-up.

 NB you'll need to adjust drivers/scsi/mpt3sas/mpt3sas_scsih.c accordingly 
if we are to move forward with this change, as it's another user of the 
SCSI_VPD_PG_LEN macro.

 Given what has been said in the discussion so far would you consider 2/3
and 3/3 unnecessary?  In the course of verifying your change I have looked 
through our code again and found that inline magic numbers are there in 
several though not all places where `scsi_get_vpd_page' gets called, so I 
think it would make sense to get rid of them all at once with a single 
self-contained change.

 Thank you for your input and the extra fix.

  Maciej