linux-4.4-rc8/arch/arm64/kernel/module.c:78: 32/64 bit problem ?

linux-4.4-rc8/arch/arm64/kernel/module.c:78: 32/64 bit problem ?

[David Binderman]

Hello there,

[linux-4.4-rc8/arch/arm64/kernel/module.c:78] -> [linux-4.4-rc8/arch/arm64/kernel/module.c:88]: (warning) Shifting 32-bit value by 64 bits is undefined behaviour. See condition at line 88.

Source code is

??? u64 imm_mask = (1 << len) - 1;
??? s64 sval = do_reloc(op, place, val);

??? switch (len) {
??? case 16:
??????? *(s16 *)place = sval;
??????? break;
??? case 32:
??????? *(s32 *)place = sval;
??????? break;
??? case 64:

So it seems that len can be 64. Suggest new code

??? u64 imm_mask = (1UL << len) - 1;
??? s64 sval = do_reloc(op, place, val);

??? switch (len) {
??? case 16:
??????? *(s16 *)place = sval;
??????? break;
??? case 32:
??????? *(s32 *)place = sval;
??????? break;
??? case 64:

Regards

David Binderman

 		 	   		  

linux-4.4-rc8/arch/arm64/kernel/module.c:78: 32/64 bit problem ?

[Will Deacon]

On Mon, Jan 04, 2016 at 08:25:30AM +0000, David Binderman wrote:
> Hello there,

Hi David,

> [linux-4.4-rc8/arch/arm64/kernel/module.c:78] ->
> [linux-4.4-rc8/arch/arm64/kernel/module.c:88]: (warning) Shifting 32-bit
> value by 64 bits is undefined behaviour. See condition at line 88.

Curious, but how are you seeing this warning? GCC is silent for me...

> Source code is
> 
> ??? u64 imm_mask = (1 << len) - 1;
> ??? s64 sval = do_reloc(op, place, val);
> 
> ??? switch (len) {
> ??? case 16:
> ??????? *(s16 *)place = sval;
> ??????? break;
> ??? case 32:
> ??????? *(s32 *)place = sval;
> ??????? break;
> ??? case 64:
> 
> So it seems that len can be 64. Suggest new code
> 
> ??? u64 imm_mask = (1UL << len) - 1;

That still ends up shifting by the width of the type when len == 64,
which is potentially still broken. We're better off using GENMASK.

Will

--->8

diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index f4bc779e62e8..266e7490e85c 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -75,7 +75,7 @@ static u64 do_reloc(enum aarch64_reloc_op reloc_op, void *place, u64 val)
 
 static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
 {
-	u64 imm_mask = (1 << len) - 1;
+	u64 imm_mask = GENMASK(len - 1, 0);
 	s64 sval = do_reloc(op, place, val);
 
 	switch (len) {

linux-4.4-rc8/arch/arm64/kernel/module.c:78: 32/64 bit problem ?

[Ard Biesheuvel]

On 4 January 2016 at 15:16, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Jan 04, 2016 at 08:25:30AM +0000, David Binderman wrote:
>> Hello there,
>
> Hi David,
>
>> [linux-4.4-rc8/arch/arm64/kernel/module.c:78] ->
>> [linux-4.4-rc8/arch/arm64/kernel/module.c:88]: (warning) Shifting 32-bit
>> value by 64 bits is undefined behaviour. See condition at line 88.
>
> Curious, but how are you seeing this warning? GCC is silent for me...
>
>> Source code is
>>
>>     u64 imm_mask = (1 << len) - 1;
>>     s64 sval = do_reloc(op, place, val);
>>
>>     switch (len) {
>>     case 16:
>>         *(s16 *)place = sval;
>>         break;
>>     case 32:
>>         *(s32 *)place = sval;
>>         break;
>>     case 64:
>>
>> So it seems that len can be 64. Suggest new code
>>
>>     u64 imm_mask = (1UL << len) - 1;
>
> That still ends up shifting by the width of the type when len == 64,
> which is potentially still broken. We're better off using GENMASK.
>

Can't we simply return from the function rather than break from the
switch statement if len == 64?
The range check does not make any sense in that case anyway.


> --->8
>
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index f4bc779e62e8..266e7490e85c 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -75,7 +75,7 @@ static u64 do_reloc(enum aarch64_reloc_op reloc_op, void *place, u64 val)
>
>  static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
>  {
> -       u64 imm_mask = (1 << len) - 1;
> +       u64 imm_mask = GENMASK(len - 1, 0);
>         s64 sval = do_reloc(op, place, val);
>
>         switch (len) {
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

linux-4.4-rc8/arch/arm64/kernel/module.c:78: 32/64 bit problem ?

[Ard Biesheuvel]

On 4 January 2016 at 15:32, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 4 January 2016 at 15:16, Will Deacon <will.deacon@arm.com> wrote:
>> On Mon, Jan 04, 2016 at 08:25:30AM +0000, David Binderman wrote:
>>> Hello there,
>>
>> Hi David,
>>
>>> [linux-4.4-rc8/arch/arm64/kernel/module.c:78] ->
>>> [linux-4.4-rc8/arch/arm64/kernel/module.c:88]: (warning) Shifting 32-bit
>>> value by 64 bits is undefined behaviour. See condition at line 88.
>>
>> Curious, but how are you seeing this warning? GCC is silent for me...
>>
>>> Source code is
>>>
>>>     u64 imm_mask = (1 << len) - 1;
>>>     s64 sval = do_reloc(op, place, val);
>>>
>>>     switch (len) {
>>>     case 16:
>>>         *(s16 *)place = sval;
>>>         break;
>>>     case 32:
>>>         *(s32 *)place = sval;
>>>         break;
>>>     case 64:
>>>
>>> So it seems that len can be 64. Suggest new code
>>>
>>>     u64 imm_mask = (1UL << len) - 1;
>>
>> That still ends up shifting by the width of the type when len == 64,
>> which is potentially still broken. We're better off using GENMASK.
>>
>
> Can't we simply return from the function rather than break from the
> switch statement if len == 64?
> The range check does not make any sense in that case anyway.
>

Or perhaps:

------------8<-----------------
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index f4bc779e62e8..fd1f4e678655 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -75,14 +75,17 @@ static u64 do_reloc(enum aarch64_reloc_op
reloc_op, void *place, u64 val)

 static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
 {
-       u64 imm_mask = (1 << len) - 1;
        s64 sval = do_reloc(op, place, val);

        switch (len) {
        case 16:
+               if (sval < S16_MIN || sval > U16_MAX)
+                       return -ERANGE;
                *(s16 *)place = sval;
                break;
        case 32:
+               if (sval < S32_MIN || sval > U32_MAX)
+                       return -ERANGE;
                *(s32 *)place = sval;
                break;
        case 64:
@@ -92,21 +95,6 @@ static int reloc_data(enum aarch64_reloc_op op,
void *place, u64 val, int len)
                pr_err("Invalid length (%d) for data relocation\n", len);
                return 0;
        }
-
-       /*
-        * Extract the upper value bits (including the sign bit) and
-        * shift them to bit 0.
-        */
-       sval = (s64)(sval & ~(imm_mask >> 1)) >> (len - 1);
-
-       /*
-        * Overflow has occurred if the value is not representable in
-        * len bits (i.e the bottom len bits are not sign-extended and
-        * the top bits are not all zero).
-        */
-       if ((u64)(sval + 1) > 2)
-               return -ERANGE;
-
        return 0;
 }

linux-4.4-rc8/arch/arm64/kernel/module.c:78: 32/64 bit problem ?

[Will Deacon]

On Mon, Jan 04, 2016 at 04:24:49PM +0100, Ard Biesheuvel wrote:
> On 4 January 2016 at 15:32, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > On 4 January 2016 at 15:16, Will Deacon <will.deacon@arm.com> wrote:
> >> On Mon, Jan 04, 2016 at 08:25:30AM +0000, David Binderman wrote:
> >>> Hello there,
> >>
> >> Hi David,
> >>
> >>> [linux-4.4-rc8/arch/arm64/kernel/module.c:78] ->
> >>> [linux-4.4-rc8/arch/arm64/kernel/module.c:88]: (warning) Shifting 32-bit
> >>> value by 64 bits is undefined behaviour. See condition at line 88.
> >>
> >> Curious, but how are you seeing this warning? GCC is silent for me...
> >>
> >>> Source code is
> >>>
> >>>     u64 imm_mask = (1 << len) - 1;
> >>>     s64 sval = do_reloc(op, place, val);
> >>>
> >>>     switch (len) {
> >>>     case 16:
> >>>         *(s16 *)place = sval;
> >>>         break;
> >>>     case 32:
> >>>         *(s32 *)place = sval;
> >>>         break;
> >>>     case 64:
> >>>
> >>> So it seems that len can be 64. Suggest new code
> >>>
> >>>     u64 imm_mask = (1UL << len) - 1;
> >>
> >> That still ends up shifting by the width of the type when len == 64,
> >> which is potentially still broken. We're better off using GENMASK.
> >>
> >
> > Can't we simply return from the function rather than break from the
> > switch statement if len == 64?
> > The range check does not make any sense in that case anyway.
> >
> 
> Or perhaps:
> 
> ------------8<-----------------
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index f4bc779e62e8..fd1f4e678655 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -75,14 +75,17 @@ static u64 do_reloc(enum aarch64_reloc_op
> reloc_op, void *place, u64 val)
> 
>  static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
>  {
> -       u64 imm_mask = (1 << len) - 1;
>         s64 sval = do_reloc(op, place, val);
> 
>         switch (len) {
>         case 16:
> +               if (sval < S16_MIN || sval > U16_MAX)
> +                       return -ERANGE;
>                 *(s16 *)place = sval;

Doesn't this break ABS relocs, which are allowed to overflow?

Will

linux-4.4-rc8/arch/arm64/kernel/module.c:78: 32/64 bit problem ?

[Will Deacon]

On Mon, Jan 04, 2016 at 03:28:41PM +0000, Will Deacon wrote:
> On Mon, Jan 04, 2016 at 04:24:49PM +0100, Ard Biesheuvel wrote:
> > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> > index f4bc779e62e8..fd1f4e678655 100644
> > --- a/arch/arm64/kernel/module.c
> > +++ b/arch/arm64/kernel/module.c
> > @@ -75,14 +75,17 @@ static u64 do_reloc(enum aarch64_reloc_op
> > reloc_op, void *place, u64 val)
> > 
> >  static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
> >  {
> > -       u64 imm_mask = (1 << len) - 1;
> >         s64 sval = do_reloc(op, place, val);
> > 
> >         switch (len) {
> >         case 16:
> > +               if (sval < S16_MIN || sval > U16_MAX)
> > +                       return -ERANGE;
> >                 *(s16 *)place = sval;
> 
> Doesn't this break ABS relocs, which are allowed to overflow?

Gah, that only applies to the 64-bit relocs. Sorry for the noise.

Will

linux-4.4-rc8/arch/arm64/kernel/module.c:78: 32/64 bit problem ?

[David Binderman]

Hello there Will,

----------------------------------------
> Curious, but how are you seeing this warning? GCC is silent for me...

Static analyser called cppcheck, available from sourceforge.

GCC will be silent for all sorts of dodgy code. clang produces more warnings, 
but I am not sure how much of the linux kernel it can compile. 
Hence use of cppcheck.


Regards

David Binderman

>
>> Source code is
>>
>>     u64 imm_mask = (1 << len) - 1;
>>     s64 sval = do_reloc(op, place, val);
>>
>>     switch (len) {
>>     case 16:
>>         *(s16 *)place = sval;
>>         break;
>>     case 32:
>>         *(s32 *)place = sval;
>>         break;
>>     case 64:
>>
>> So it seems that len can be 64. Suggest new code
>>
>>     u64 imm_mask = (1UL << len) - 1;
>
> That still ends up shifting by the width of the type when len == 64,
> which is potentially still broken. We're better off using GENMASK.
>
> Will
>
> --->8
>
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index f4bc779e62e8..266e7490e85c 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -75,7 +75,7 @@ static u64 do_reloc(enum aarch64_reloc_op reloc_op, void *place, u64 val)
>
> static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
> {
> - u64 imm_mask = (1 << len) - 1;
> + u64 imm_mask = GENMASK(len - 1, 0);
> s64 sval = do_reloc(op, place, val);
>
> switch (len) {