Re: [PATCH v2] kbuild: treat char as always unsigned

Re: [PATCH v2] kbuild: treat char as always unsigned

[Jason A. Donenfeld]

Hi Linus,

On Mon, Oct 24, 2022 at 7:11 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> IOW, I don't think these are 6.1 material as some kind of obvious
> fixes, at least not without driver author acks.

Right, these are posted to the authors and maintainers to look at.
Maybe they punt them until 6.2 which would be fine too.

> On Mon, Oct 24, 2022 at 9:34 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Some of those may need more thought. For example, that first one:
>
> > https://lore.kernel.org/all/20221024163005.536097-1-Jason@zx2c4.com
>
> looks just *strange*. As far as I can tell, no other wireless drivers
> do any sign checks at all.
>
> Now, I didn't really look around a lot, but looking at a few other
> SIOCSIWESSID users, most don't even seem to treat it as a string at
> all, but as just a byte dump (so memcpy() instead of strncpy())
>
> As far as I know, there are no actual rules for SSID character sets,
> and while using utf-8 or something else might cause interoperability
> problems, this driver seems to be just confused. If you want to check
> for "printable characters", that check is still wrong.
>
> So I don't think this is a "assume char is signed" issue. I think this
> is a "driver is confused" issue.

Yea I had a few versions of this. In one of them, I changed `char
*extra` throughout the wireless stack into `s8 *extra` and in another
`u8 *extra`, after realizing they're mostly just bags of bits. But
that seemed pretty invasive when, indeed, this staging driver is just
a little screwy.

So perhaps the right fix is to just kill that whole snippet? Kalle - opinions?

Jason

Re: [PATCH v2] kbuild: treat char as always unsigned

[Kalle Valo]

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> Hi Linus,
>
> On Mon, Oct 24, 2022 at 7:11 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> IOW, I don't think these are 6.1 material as some kind of obvious
>> fixes, at least not without driver author acks.
>
> Right, these are posted to the authors and maintainers to look at.
> Maybe they punt them until 6.2 which would be fine too.
>
>> On Mon, Oct 24, 2022 at 9:34 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> Some of those may need more thought. For example, that first one:
>>
>> > https://lore.kernel.org/all/20221024163005.536097-1-Jason@zx2c4.com
>>
>> looks just *strange*. As far as I can tell, no other wireless drivers
>> do any sign checks at all.
>>
>> Now, I didn't really look around a lot, but looking at a few other
>> SIOCSIWESSID users, most don't even seem to treat it as a string at
>> all, but as just a byte dump (so memcpy() instead of strncpy())

Yes, SSID should be handled as a byte array with a specified length.
Back in the day some badly written code treated it as string but luckily
it's rare now.

>> As far as I know, there are no actual rules for SSID character sets,
>> and while using utf-8 or something else might cause interoperability
>> problems, this driver seems to be just confused. If you want to check
>> for "printable characters", that check is still wrong.
>>
>> So I don't think this is a "assume char is signed" issue. I think this
>> is a "driver is confused" issue.
>
> Yea I had a few versions of this. In one of them, I changed `char
> *extra` throughout the wireless stack into `s8 *extra` and in another
> `u8 *extra`, after realizing they're mostly just bags of bits. But
> that seemed pretty invasive when, indeed, this staging driver is just
> a little screwy.
>
> So perhaps the right fix is to just kill that whole snippet? Kalle - opinions?

I would also remove the whole 'extra[i] < 0', seems like a pointless
check to me. And I see that you already submitted v2, good.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches