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 at linaro.org> wrote:
> > On 4 January 2016 at 15:16, Will Deacon <will.deacon at 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