Re: [PATCH] unlzma: avoid UB shift

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.03.2026 17:24, Mykola Kvach wrote:
> On Tue, Mar 24, 2026 at 5:27 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> Shifting signed quantities has restrictions. Since the wrapping macro of
>> read_int() type-casts the result anyway, switch function return type as
>> well as the local variable to the corresponding unsigned type.
>>
>> Reported-by: Kamil Frankowicz <kamil.frankowicz@xxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> We've inherited that code from Linux, and the same code still exists
>> there. As I'm entirely uncertain whether they would even care, I'd prefer
>> to not take the route of posting a patch against Linux first.

As following from this, imo we want to diverge from the original as little
as possible. IOW ...

>> --- a/xen/common/unlzma.c
>> +++ b/xen/common/unlzma.c
>> @@ -30,10 +30,10 @@
>>
>>  #include "decompress.h"
>>
>> -static long long __init read_int(unsigned char *ptr, int size)
>> +static unsigned long long __init read_int(unsigned char *ptr, int size)
> 
> nit: Since we're touching read_int() anyway, would it make sense to also
> tighten the helper's interface, i.e. make ptr const and use size_t for
> size?
> 
> That would better match the actual usage: the buffer is only read from,
> and size is really a byte count, usually coming from sizeof().
> 
>>  {
>>         int i;
>> -       long long ret = 0;
>> +       unsigned long long ret = 0;
>>
>>         for (i = 0; i < size; i++)
>>                 ret = (ret << 8) | ptr[size-i-1];
> 
> Separately, the loop could also be written without the temporary i, using
> a simpler reverse traversal, for example:
> 
> while ( size )
>     ret = (ret << 8) | ptr[--size];

... I'd suggest that you submit patches against Linux, for us to then
re-sync.

Jan