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

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

[Milan Broz]

Hello,

I tried to extend USB storage for the passthrough of Opal
security commands, and some adapters are clearly "not perfect".
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... :)

Thanks,
Milan

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

[Alan Stern]

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?

>  and some adapters are clearly "not perfect".

Which ones?

> 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.

Alan Stern

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

[Milan Broz]

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

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

[Alan Stern]

On Sun, Aug 27, 2023 at 06:45:55PM +0200, Milan Broz wrote:
> 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.

It sounds like the sort of thing that should be reported as a bug to 
Realtek.  I can't imagine their customers would be very happy about this 
behavior.

> 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.

Certainly.  We knew this was bound to come up eventually.

> > > 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.

I don't think it will be necessary to change all those macros, just the 
ones in usual_tables.c.  And to create the new table containing the 
actual flag values, of course.

There will also have to be a new argument to usb_stor_probe1() 
specifying whether the id->driver_info field is standard (i.e., it 
contains the flags directly) or is one of the new indirect index values.  

And you'll have to figure out a comparable change to the dynamic device 
ID table mechanism.

(If you want to be really fancy about it, you could design things in 
such a way that the indirect flags approach is used only on 32-bit 
systems.  64-bit systems can put the new flag bits directly into the 
driver_info field.  However, it's probably best not to worry about this 
initially.)
 
Alan Stern

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

[Milan Broz]

On 8/27/23 20:55, Alan Stern wrote:

...

>>>> 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.
> 
> I don't think it will be necessary to change all those macros, just the
> ones in usual_tables.c.  And to create the new table containing the
> actual flag values, of course.
> 
> There will also have to be a new argument to usb_stor_probe1()
> specifying whether the id->driver_info field is standard (i.e., it
> contains the flags directly) or is one of the new indirect index values.
> 
> And you'll have to figure out a comparable change to the dynamic device
> ID table mechanism.
> 
> (If you want to be really fancy about it, you could design things in
> such a way that the indirect flags approach is used only on 32-bit
> systems.  64-bit systems can put the new flag bits directly into the
> driver_info field.  However, it's probably best not to worry about this
> initially.)

Hi Alan,

So, I really tried this approach, spent more time on in than I expected, but
produced working code... that I am really not proud of :-]
(Thus avoiding to send it here, for now.)

I pushed it to my dm-cryptsetup branch here
https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/log/?h=dm-cryptsetup

The last patch is the reason why I need it, just for reference.
More comments in the patch headers.

Could you please check it if it is *really* what we want?
If so, I'll rebase it for usb next tree and send as a patchset.

But the macro magic is crazy... and I really did not find the better way.

Anyway, it also uncovered some problems
  - some macros need to be changed a little bit, there is even old one unused
  - duplicity of entries in UAS and mass-storage are strange (and complicates
    the approach).
    I guess the sorting is intentionally that mass-storage is included
    before UAS, so the mass-storage quirk is found as the first (for non-UAS).
    (While UAS drive includes only own header.)
- the patch significantly increases size of module for 32bit
   (64bit system use the direct flag store approach)
- I stored a pointer to the flags array, not the index. Perhaps it should be
   index only (trivial change, though).

Thanks,
Milan

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

[Alan Stern]

On Wed, Aug 30, 2023 at 10:39:07PM +0200, Milan Broz wrote:
> On 8/27/23 20:55, Alan Stern wrote:
> 
> ...
> 
> > > > > 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.
> > 
> > I don't think it will be necessary to change all those macros, just the
> > ones in usual_tables.c.  And to create the new table containing the
> > actual flag values, of course.
> > 
> > There will also have to be a new argument to usb_stor_probe1()
> > specifying whether the id->driver_info field is standard (i.e., it
> > contains the flags directly) or is one of the new indirect index values.
> > 
> > And you'll have to figure out a comparable change to the dynamic device
> > ID table mechanism.
> > 
> > (If you want to be really fancy about it, you could design things in
> > such a way that the indirect flags approach is used only on 32-bit
> > systems.  64-bit systems can put the new flag bits directly into the
> > driver_info field.  However, it's probably best not to worry about this
> > initially.)
> 
> Hi Alan,
> 
> So, I really tried this approach, spent more time on in than I expected, but
> produced working code... that I am really not proud of :-]
> (Thus avoiding to send it here, for now.)
> 
> I pushed it to my dm-cryptsetup branch here
> https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/log/?h=dm-cryptsetup

I haven't had a chance to look at your changes yet, and I might not get 
around to it for a while.

> The last patch is the reason why I need it, just for reference.
> More comments in the patch headers.
> 
> Could you please check it if it is *really* what we want?
> If so, I'll rebase it for usb next tree and send as a patchset.
> 
> But the macro magic is crazy... and I really did not find the better way.

Another possibility is to create a simple pre-processor program that 
would read the unusual_devs files and massage the information into the 
form the driver will want.  Such a program could do things that the C 
preprocessor isn't capable of.

For example, with this approach you could keep the original flag bits in 
positions 0 - 30, and reserve bit 31 as an indicator that there are 
additional flags stored in an "overflow" table entry.  This extra table 
could be relatively short, since it would need to contain only a small 
number of entries.  I can't think of any way to get the C preprocessor 
to do this, but it would be easy to have a separate program do it.

> Anyway, it also uncovered some problems
>  - some macros need to be changed a little bit, there is even old one unused

It doesn't hurt to have someone with fresh eyes taking a look at this.  :-)

>  - duplicity of entries in UAS and mass-storage are strange (and complicates
>    the approach).
>    I guess the sorting is intentionally that mass-storage is included
>    before UAS, so the mass-storage quirk is found as the first (for non-UAS).
>    (While UAS drive includes only own header.)

It's a little tricky because the two drivers need to be aware, to some 
extent, of which devices either prefer to use UAS or can't work with UAS.

> - the patch significantly increases size of module for 32bit
>   (64bit system use the direct flag store approach)

The "overflow table" approach would help a lot with this.

> - I stored a pointer to the flags array, not the index. Perhaps it should be
>   index only (trivial change, though).

Alan Stern