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]

Date:   Wed, 30 Aug 2023 22:39:07 +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 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