linux-kernel - Re: usb-storage: how to extend quirks flags to 64bit?

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: <34aa2eea-5417-4e46-91a0-aac1a46a84cf@gmail.com>
Date:   Sun, 27 Aug 2023 18:45:55 +0200
From:   Milan Broz <gmazyland@...il.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, usb-storage@...ts.one-eyed-alien.net,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: usb-storage: how to extend quirks flags to 64bit?

On 8/27/23 17:50, Alan Stern wrote:
> On Sun, Aug 27, 2023 at 11:32:05AM +0200, Milan Broz wrote:
>> Hello,
>>
>> I tried to extend USB storage for the passthrough of Opal
>> security commands,
> 
> What sort of changes are needed?  Where is this passthrough mechanism
> documented?

We are currently adding support for optional OPAL hw encryption to
cryptsetup/LUKS2 (that uses kernel OPAL ioctl interface) and I tried
to make USB adapters to work with it too.

I'll send RFC patchset (it is quite simple) where I explain it in detail.
The patch for USB storage is actually one liner, the rest is in SCSI driver :)

Basically, USB adapters (not supporting UAS) cannot work as
required SCSI SECURITY IN/OUT SCSI commands do not work here.

But we can use ATA12 pass-thru (as used with original sedutils
and some other tools we used in research; it is a documented feature).
It works once ATA12 wrapper is added to block layer and USB storage enables
the "security_supported" bit.

> 
>>   and some adapters are clearly "not perfect".
> 
> Which ones?

Namely Realtek 9210 family (NVME to USB bridge). Everything OPAL related
works, but the adapter always set write-protected bit for the whole
drive (even if OPAL locking range is just covering part of the disk).

I spent quite a lot time trying new firmware versions - this issue is
still there.

On the other side, many other USB to SATA bridges works nicely.
I think this is the exact situation where we should set a new quirks flag
to disable it. (The nasty thing is that for unbricking it you need PSID reset
- PSID is a number written on the label of the drive - followed by physical
disconnect for recovery.)

Anyway, I intentionally sent this 32bit flags question separately as it
is actually a generic issue - we are just out of flag space now...

Even if the patches mentioned above are rejected, someone will need
a new flag for something else later.

>> I would need to introduce a new quirks flag to turn it off.
>>
>> Seems that we are already out of quirks flags on 32bit
>> for usb storage - in usb_usual.h the last entry in mainline is
>>    US_FLAG(SENSE_AFTER_SYNC, 0x80000000)
>>
>> Adding a new flag will work for 64-bit systems but not
>> for platforms with 32-bit unsigned long like i686.
>>
>> How do we allow new flag definitions?
>>
>> Struct us_data fflags can be made 64bit (defined in
>> drivers/usb/storage/usb.h), but the major problem is that these
>> are transferred through the generic driver_info field
>> defined in linux/mod_devicetable.h as unsigned long).
>> Making this 64bit is IMO an extensive API change (if even possible).
>> I guess this is not the way to go.
>>
>> Could USB maintainers please help to advise what is the correct
>> solution? I am not familiar with the USB driver model here
>> and I see no easy way how it can be solved by a trivial static
>> allocation inside the USB storage driver.
>>
>> Someone will need a new quirks flag in the future anyway... :)
> 
> I can think of only one way to accomplish this on 32-bit systems: Change
> the driver_info field from a bit array to an index into a static table
> of 64-bit flags values.  Each unusual_devs structure would have its own
> entry in this table.  As far as I can tell, the other unusual_*.h tables
> could retain their current driver_info interpretations, since no new
> quirk bits are likely to be relevant to them.
> 
> Making this change would be an awkward nuisance, but it should be
> doable.

Hm, yes, thanks for the idea,that is a possible solution.
It will need to modify all unusual macros, though. Just I am not sure I want
to spent time patching all the drivers as I have not way how to test it.

Thanks,
Milan