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@cert.pl>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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.

--- 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)
 {
 	int i;
-	long long ret = 0;
+	unsigned long long ret = 0;
 
 	for (i = 0; i < size; i++)
 		ret = (ret << 8) | ptr[size-i-1];

On 24/03/2026 3:26 pm, Jan Beulich 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@cert.pl>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

On Tue, Mar 24, 2026 at 5:27 PM Jan Beulich <jbeulich@suse.com> 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@cert.pl>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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.
>
> --- 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];

>

Best regards,
Mykola

On 24.03.2026 17:24, Mykola Kvach wrote:
> On Tue, Mar 24, 2026 at 5:27 PM Jan Beulich <jbeulich@suse.com> 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@cert.pl>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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

On 24/03/2026 4:24 pm, Mykola Kvach wrote:
> On Tue, Mar 24, 2026 at 5:27 PM Jan Beulich <jbeulich@suse.com> 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@cert.pl>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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.
>>
>> --- 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?

No, not mixed into a patch which is fixing a real bug.  Cleanup can come
later.

This code is vendored from Linux.  For better or worse we try not to
deviate, although that is going to have to change at some point.

~Andrew