Linux-m68k Archive on lore.kernel.org
 help / color / Atom feed
* Re: Build regressions/improvements in v5.5-rc2
       [not found] <20191216083658.21386-1-geert@linux-m68k.org>
@ 2019-12-16  8:49 ` Geert Uytterhoeven
  2019-12-17  0:56   ` Greg Ungerer
  0 siblings, 1 reply; 2+ messages in thread
From: Geert Uytterhoeven @ 2019-12-16  8:49 UTC (permalink / raw)
  To: Greg Ungerer; +Cc: Linux Kernel Mailing List, linux-m68k

Hi Greg,

On Mon, Dec 16, 2019 at 9:38 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Below is the list of build error/warning regressions/improvements in
> v5.5-rc2[1] compared to v5.4[2].

> [1] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/d1eef1c619749b2a57e514a3fa67d9a516ffa919/ (all 232 configs)
> [2] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/219d54332a09e8d8741c1e1982f5eae56099de85/ (all 232 configs)

>   + /kisskb/src/arch/m68k/include/asm/string.h: warning: '__builtin_memcpy' forming offset [3, 4] is out of the bounds [0, 2] of object '__gu_val' with type 'short unsigned int' [-Warray-bounds]:  => 72:25

The upgrade from gcc 4.6.3 to 81.0 seems to have revealed a potential
issue in get_user() in arch/m68k/include/asm/uaccess_no.h.

        #define get_user(x, ptr)                                        \
        ({                                                              \
            int __gu_err = 0;                                           \
            typeof(x) __gu_val = 0;                                     \
            ^^^^^^^^^
            This is the type of the destination

            switch (sizeof(*(ptr))) {                                   \
            case 1:                                                     \
                __get_user_asm(__gu_err, __gu_val, ptr, b, "=d");       \
                break;                                                  \
            case 2:                                                     \
                __get_user_asm(__gu_err, __gu_val, ptr, w, "=r");       \
                break;                                                  \
            case 4:                                                     \
                __get_user_asm(__gu_err, __gu_val, ptr, l, "=r");       \
                break;                                                  \
            case 8:                                                     \
                memcpy((void *) &__gu_val, ptr, sizeof (*(ptr)));       \
                break;                                                  \
            default:                                                    \
                __gu_val = 0;                                           \
                __gu_err = __get_user_bad();                            \
                break;                                                  \
            }                                                           \
            (x) = (typeof(*(ptr))) __gu_val;                            \
            __gu_err;                                                   \
        })

ext2_ioctl() calls this like

        unsigned short rsv_window_size;
        if (get_user(rsv_window_size, (int __user *)arg)) { ... }

So a 32-bit value is being copied to an unsigned short value, leading
to the warning (for the memcpy() in the non-taken "case 8" branch).

Fortunately the compiler emits a register move for this, so no real harm
is done:

        | fs/ext2/ioctl.c:123:          if (get_user(rsv_window_size,
(int __user *)arg))
                move.l 48(%sp),%a0      | arg,
        #APP
        | 123 "fs/ext2/ioctl.c" 1
                movel (%a0),%d2 | *arg.32_43, __gu_val

The corresponding code in arch/m68k/include/asm/uaccess_mm.h
uses a temporary __gu_val of the right sized type based on the source
type to avoid this.


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: Build regressions/improvements in v5.5-rc2
  2019-12-16  8:49 ` Build regressions/improvements in v5.5-rc2 Geert Uytterhoeven
@ 2019-12-17  0:56   ` Greg Ungerer
  0 siblings, 0 replies; 2+ messages in thread
From: Greg Ungerer @ 2019-12-17  0:56 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Linux Kernel Mailing List, linux-m68k

Hi Geert,

On 16/12/19 6:49 pm, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Mon, Dec 16, 2019 at 9:38 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Below is the list of build error/warning regressions/improvements in
>> v5.5-rc2[1] compared to v5.4[2].
> 
>> [1] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/d1eef1c619749b2a57e514a3fa67d9a516ffa919/ (all 232 configs)
>> [2] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/219d54332a09e8d8741c1e1982f5eae56099de85/ (all 232 configs)
> 
>>    + /kisskb/src/arch/m68k/include/asm/string.h: warning: '__builtin_memcpy' forming offset [3, 4] is out of the bounds [0, 2] of object '__gu_val' with type 'short unsigned int' [-Warray-bounds]:  => 72:25
> 
> The upgrade from gcc 4.6.3 to 81.0 seems to have revealed a potential
> issue in get_user() in arch/m68k/include/asm/uaccess_no.h.
> 
>          #define get_user(x, ptr)                                        \
>          ({                                                              \
>              int __gu_err = 0;                                           \
>              typeof(x) __gu_val = 0;                                     \
>              ^^^^^^^^^
>              This is the type of the destination
> 
>              switch (sizeof(*(ptr))) {                                   \
>              case 1:                                                     \
>                  __get_user_asm(__gu_err, __gu_val, ptr, b, "=d");       \
>                  break;                                                  \
>              case 2:                                                     \
>                  __get_user_asm(__gu_err, __gu_val, ptr, w, "=r");       \
>                  break;                                                  \
>              case 4:                                                     \
>                  __get_user_asm(__gu_err, __gu_val, ptr, l, "=r");       \
>                  break;                                                  \
>              case 8:                                                     \
>                  memcpy((void *) &__gu_val, ptr, sizeof (*(ptr)));       \
>                  break;                                                  \
>              default:                                                    \
>                  __gu_val = 0;                                           \
>                  __gu_err = __get_user_bad();                            \
>                  break;                                                  \
>              }                                                           \
>              (x) = (typeof(*(ptr))) __gu_val;                            \
>              __gu_err;                                                   \
>          })
> 
> ext2_ioctl() calls this like
> 
>          unsigned short rsv_window_size;
>          if (get_user(rsv_window_size, (int __user *)arg)) { ... }
> 
> So a 32-bit value is being copied to an unsigned short value, leading
> to the warning (for the memcpy() in the non-taken "case 8" branch).
> 
> Fortunately the compiler emits a register move for this, so no real harm
> is done:
> 
>          | fs/ext2/ioctl.c:123:          if (get_user(rsv_window_size,
> (int __user *)arg))
>                  move.l 48(%sp),%a0      | arg,
>          #APP
>          | 123 "fs/ext2/ioctl.c" 1
>                  movel (%a0),%d2 | *arg.32_43, __gu_val
> 
> The corresponding code in arch/m68k/include/asm/uaccess_mm.h
> uses a temporary __gu_val of the right sized type based on the source
> type to avoid this.

Yeah, your right. I will put a change together to fix.

Regards
Greg


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20191216083658.21386-1-geert@linux-m68k.org>
2019-12-16  8:49 ` Build regressions/improvements in v5.5-rc2 Geert Uytterhoeven
2019-12-17  0:56   ` Greg Ungerer

Linux-m68k Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-m68k/0 linux-m68k/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-m68k linux-m68k/ https://lore.kernel.org/linux-m68k \
		linux-m68k@vger.kernel.org linux-m68k@lists.linux-m68k.org
	public-inbox-index linux-m68k

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-m68k


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git