linux-kernel - Re: [PATCH v3 3/5] platform/x86: think-lmi: Correct NVME password handling

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]

Date:   Mon, 29 May 2023 18:41:45 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Mark Pearson <mpearson-lenovo@...ebb.ca>
cc:     Hans de Goede <hdegoede@...hat.com>,
        "markgross@...nel.org" <markgross@...nel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/5] platform/x86: think-lmi: Correct NVME password
 handling

On Mon, 29 May 2023, Mark Pearson wrote:
> On Mon, May 29, 2023, at 8:03 AM, Ilpo Järvinen wrote:
> > On Fri, 26 May 2023, Mark Pearson wrote:
> >
> >> NVME passwords identifier have been standardised across the Lenovo
> >> systems and now use udrp and adrp (user and admin level) instead of
> >> unvp and mnvp.
> >> 
> >> This should apparently be backwards compatible.
> >> 
> >> Also cleaned up so the index is set to a default of 1 rather than 0
> >> as this just makes more sense (there is no device 0).
> >
> > These two sound entirely separate changes. If that's the case, please 
> > make own patch from the send change.
> 
> Ack. It was all related to the index setting and seemed trivial so I 
> lumped together but I can split. This patch series is turning into a 
> good learning exercise for my git skills :) (which are limited)
>
> > Hmm, index_store() still allows 0, is that also related here? Please check 
> > also ABI documentation as index default seems to be mentioned there as 
> > well.
> >
> 
> I'd rather not limit it so 0 isn't allowed in case our BIOS team does 
> something weird in the future; but right now 1 is the default so it 
> makes more sense.

Sure, do what you feel makes sense here. I was just pointing out the 
perceived inconsistency in case it wasn't intentional.

It might be useful to add one sentence into changelog about the reasoning 
so it can be found easier later on (effectively the paragraph you wrote 
above with small tweaks is enough I think).


-- 
 i.