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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 10 Feb 2019 21:25:55 +0100 (CET)
From:   David Kozub <zub@...ux.fjfi.cvut.cz>
To:     Scott Bauer <sbauer@...donthack.me>
cc:     "Derrick, Jonathan" <jonathan.derrick@...el.com>,
        "hch@...radead.org" <hch@...radead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "jonas.rabenstein@...dium.uni-erlangen.de" 
        <jonas.rabenstein@...dium.uni-erlangen.de>,
        "axboe@...nel.dk" <axboe@...nel.dk>
Subject: Re: [PATCH v4 10/16] block: sed-opal: add ioctl for done-mark of
 shadow mbr

On Sun, 10 Feb 2019, Scott Bauer wrote:

> On Fri, Feb 08, 2019 at 12:44:14AM +0000, Derrick, Jonathan wrote:
>> On Thu, 2019-02-07 at 23:56 +0100, David Kozub wrote:
>>> On Mon, 4 Feb 2019, Christoph Hellwig wrote:
>>>>>  static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
>>>>>  					  struct opal_mbr_data *opal_mbr)
>>>>>  {
>>>>> +	u8 token = opal_mbr->enable_disable == OPAL_MBR_ENABLE
>>>>> +		? OPAL_TRUE : OPAL_FALSE;
>>>>>  	const struct opal_step mbr_steps[] = {
>>>>>  		{ opal_discovery0, },
>>>>>  		{ start_admin1LSP_opal_session, &opal_mbr->key },
>>>>> -		{ set_mbr_done, &opal_mbr->enable_disable },
>>>>> +		{ set_mbr_done, &token },
>
>>> Am I missing something here? This seems wrong to me. And I think this
>>> patch actually changes it by introducing:
>>>
>>> +    u8 token = opal_mbr->enable_disable == OPAL_MBR_ENABLE
>>> +            ? OPAL_TRUE : OPAL_FALSE;
>>>
>>> which is essentially a negation (map 0 to 1 and 1 to 0).
>
> Agreed the original code did the opposite of what the user wanted, looks like
> when I authored it I messed up that enum which set everything off.
>
>
>
>>> With regard to the new IOC_OPAL_MBR_STATUS: I find the usage of
>>> OPAL_MBR_ENABLE/DISABLE for this confusing: what should passing
>>> OPAL_MBR_ENABLE do? Should it enable the shadow MBR? Or should it
>>> enable the MBR-done flag? I think the implementation in this patch
>>> interprets OPAL_MBR_ENABLE as 'set the "done" flag to true', thus hiding
>>> the shadow MBR. But this is not obvious looking at the IOCTL name.
>
> For the new ioctl I think we should just add a new enum with the correct
> nomenclature.  So OPAL_MBR_DONE, OPAL_MBR_NOT_DONE.
>
>
>> In order to keep the userspace interface consistent, I'll ACK your
>> change in this patch, unless Scott can fill me in on why this looks
>> wrong but is actually right.
>
> I think it is just wrong.
>
>
>>
>> We have 7 bytes in the opal_mbr_data struct we could use for DONE/NOT
>> DONE. I'm not sure how to go about keeping it consistent with old uapi,
>> although arguably opal_enable_disable_shadow_mbr is already doing the
>> wrong thing with DONE and ENABLE so it's low impact.
>
> Can we keep the old mbr struct the same and just add a new struct with new enums
> for the new done ioctl? I think this will keep the new ioctl cleaner instead
> of trying to apply older, some what incorrectly named, enums.

I like this proposal, I'll try to implement it. Although currently I plan 
to first re-submit the cleanup parts, as suggested by Christoph[1]. So 
this will happen after that.

> Lastly someone will need to backport his
>
>>>>> +       u8 token = opal_mbr->enable_disable == OPAL_MBR_ENABLE
>>>>> +               ? OPAL_TRUE : OPAL_FALSE;
>
> to stable so we can fix up my broken coding in older kernels.
>
>
> I can do that or, if David wants to do that that's fine... just want to coordinate.

I'm quite busy juggling patches in this series. If you can find the time, 
please do it.

Best regards,
David

[1] https://lore.kernel.org/lkml/20190204150415.GO31132@infradead.org/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ