RE: [PATCH] evh_bytechan: fix out of bounds accesses

-----Original Message-----
From: Linuxppc-dev <linuxppc-dev-
bounces+laurentiu.tudor=nxp.com@lists.ozlabs.org> On Behalf Of Stephen
Rothwell
Sent: Tuesday, January 14, 2020 8:32 AM
To: Timur Tabi <timur@kernel.org>
Cc: b08248@gmail.com; Greg Kroah-Hartman <gregkh@linuxfoundation.org>;
Jiri Slaby [off-list ref]; York Sun [off-list ref]; PowerPC Mailing
List [off-list ref]; swood@redhat.com
Subject: Re: [PATCH] evh_bytechan: fix out of bounds accesses

Hi Timur,

On Mon, 13 Jan 2020 19:10:11 -0600 Timur Tabi [off-list ref] wrote:

quoted

On 1/13/20 2:25 PM, Stephen Rothwell wrote:

quoted

The problem is not really the declaration, the problem is that
ev_byte_channel_send always accesses 16 bytes from the buffer and it

is

quoted

quoted

not always passed a buffer that long (in one case it is passed a
pointer to a single byte).  So the alternative to the memcpy approach

I

quoted

quoted

have take is to complicate ev_byte_channel_send so that only accesses
count bytes from the buffer.

Ah, I see now.  This is all coming back to me.

I would prefer that ev_byte_channel_send() is updated to access only
'count' bytes.  If that means adding a memcpy to the
ev_byte_channel_send() itself, then so be it.  Trying to figure out how
to stuff n bytes into 4 32-bit registers is probably not worth the

effort.

Like this (I have compile tested this):

From: Stephen Rothwell <redacted>
Date: Thu, 9 Jan 2020 18:23:48 +1100
Subject: [PATCH v2] evh_bytechan: fix out of bounds accesses

ev_byte_channel_send() assumes that its third argument is a 16 byte array.
Some places where it is called it may not be (or we can't easily tell
if it is).  Newer compilers have started producing warnings about this,
so copy the bytes to send into a local array if the passed length is
to short.

Since all the calls of ev_byte_channel_send() are in one file, lets move
it there from the header file and let the compiler decide if it wants
to inline it.

The warnings are:

In file included from include/linux/byteorder/big_endian.h:5,
                 from arch/powerpc/include/uapi/asm/byteorder.h:14,
                 from include/asm-generic/bitops/le.h:6,
                 from arch/powerpc/include/asm/bitops.h:250,
                 from include/linux/bitops.h:29,
                 from include/linux/kernel.h:12,
                 from include/asm-generic/bug.h:19,
                 from arch/powerpc/include/asm/bug.h:109,
                 from include/linux/bug.h:5,
                 from include/linux/mmdebug.h:5,
                 from include/linux/gfp.h:5,
                 from include/linux/slab.h:15,
                 from drivers/tty/ehv_bytechan.c:24:
drivers/tty/ehv_bytechan.c: In function ‘ehv_bc_udbg_putc’:
arch/powerpc/include/asm/epapr_hcalls.h:298:20: warning: array subscript 1
is outside array bounds of ‘const char[1]’ [-Warray-bounds]
  298 |  r6 = be32_to_cpu(p[1]);
include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of
macro ‘__be32_to_cpu’
   40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
      |                                                   ^
arch/powerpc/include/asm/epapr_hcalls.h:298:7: note: in expansion of macro
‘be32_to_cpu’
  298 |  r6 = be32_to_cpu(p[1]);
      |       ^~~~~~~~~~~
drivers/tty/ehv_bytechan.c:166:13: note: while referencing ‘data’
  166 | static void ehv_bc_udbg_putc(char c)
      |             ^~~~~~~~~~~~~~~~
In file included from include/linux/byteorder/big_endian.h:5,
                 from arch/powerpc/include/uapi/asm/byteorder.h:14,
                 from include/asm-generic/bitops/le.h:6,
                 from arch/powerpc/include/asm/bitops.h:250,
                 from include/linux/bitops.h:29,
                 from include/linux/kernel.h:12,
                 from include/asm-generic/bug.h:19,
                 from arch/powerpc/include/asm/bug.h:109,
                 from include/linux/bug.h:5,
                 from include/linux/mmdebug.h:5,
                 from include/linux/gfp.h:5,
                 from include/linux/slab.h:15,
                 from drivers/tty/ehv_bytechan.c:24:
arch/powerpc/include/asm/epapr_hcalls.h:299:20: warning: array subscript 2
is outside array bounds of ‘const char[1]’ [-Warray-bounds]
  299 |  r7 = be32_to_cpu(p[2]);
include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of
macro ‘__be32_to_cpu’
   40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
      |                                                   ^
arch/powerpc/include/asm/epapr_hcalls.h:299:7: note: in expansion of macro
‘be32_to_cpu’
  299 |  r7 = be32_to_cpu(p[2]);
      |       ^~~~~~~~~~~
drivers/tty/ehv_bytechan.c:166:13: note: while referencing ‘data’
  166 | static void ehv_bc_udbg_putc(char c)
      |             ^~~~~~~~~~~~~~~~
In file included from include/linux/byteorder/big_endian.h:5,
                 from arch/powerpc/include/uapi/asm/byteorder.h:14,
                 from include/asm-generic/bitops/le.h:6,
                 from arch/powerpc/include/asm/bitops.h:250,
                 from include/linux/bitops.h:29,
                 from include/linux/kernel.h:12,
                 from include/asm-generic/bug.h:19,
                 from arch/powerpc/include/asm/bug.h:109,
                 from include/linux/bug.h:5,
                 from include/linux/mmdebug.h:5,
                 from include/linux/gfp.h:5,
                 from include/linux/slab.h:15,
                 from drivers/tty/ehv_bytechan.c:24:
arch/powerpc/include/asm/epapr_hcalls.h:300:20: warning: array subscript 3
is outside array bounds of ‘const char[1]’ [-Warray-bounds]
  300 |  r8 = be32_to_cpu(p[3]);
include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of
macro ‘__be32_to_cpu’
   40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
      |                                                   ^
arch/powerpc/include/asm/epapr_hcalls.h:300:7: note: in expansion of macro
‘be32_to_cpu’
  300 |  r8 = be32_to_cpu(p[3]);
      |       ^~~~~~~~~~~
drivers/tty/ehv_bytechan.c:166:13: note: while referencing ‘data’
  166 | static void ehv_bc_udbg_putc(char c)
      |             ^~~~~~~~~~~~~~~~
In file included from include/linux/byteorder/big_endian.h:5,
                 from arch/powerpc/include/uapi/asm/byteorder.h:14,
                 from include/asm-generic/bitops/le.h:6,
                 from arch/powerpc/include/asm/bitops.h:250,
                 from include/linux/bitops.h:29,
                 from include/linux/kernel.h:12,
                 from include/asm-generic/bug.h:19,
                 from arch/powerpc/include/asm/bug.h:109,
                 from include/linux/bug.h:5,
                 from include/linux/mmdebug.h:5,
                 from include/linux/gfp.h:5,
                 from include/linux/slab.h:15,
                 from drivers/tty/ehv_bytechan.c:24:
arch/powerpc/include/asm/epapr_hcalls.h:298:20: warning: array subscript 1
is outside array bounds of ‘const char[1]’ [-Warray-bounds]
  298 |  r6 = be32_to_cpu(p[1]);
include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of
macro ‘__be32_to_cpu’
   40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
      |                                                   ^
arch/powerpc/include/asm/epapr_hcalls.h:298:7: note: in expansion of macro
‘be32_to_cpu’
  298 |  r6 = be32_to_cpu(p[1]);
      |       ^~~~~~~~~~~
drivers/tty/ehv_bytechan.c:166:13: note: while referencing ‘data’
  166 | static void ehv_bc_udbg_putc(char c)
      |             ^~~~~~~~~~~~~~~~
In file included from include/linux/byteorder/big_endian.h:5,
                 from arch/powerpc/include/uapi/asm/byteorder.h:14,
                 from include/asm-generic/bitops/le.h:6,
                 from arch/powerpc/include/asm/bitops.h:250,
                 from include/linux/bitops.h:29,
                 from include/linux/kernel.h:12,
                 from include/asm-generic/bug.h:19,
                 from arch/powerpc/include/asm/bug.h:109,
                 from include/linux/bug.h:5,
                 from include/linux/mmdebug.h:5,
                 from include/linux/gfp.h:5,
                 from include/linux/slab.h:15,
                 from drivers/tty/ehv_bytechan.c:24:
arch/powerpc/include/asm/epapr_hcalls.h:299:20: warning: array subscript 2
is outside array bounds of ‘const char[1]’ [-Warray-bounds]
  299 |  r7 = be32_to_cpu(p[2]);
include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of
macro ‘__be32_to_cpu’
   40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
      |                                                   ^
arch/powerpc/include/asm/epapr_hcalls.h:299:7: note: in expansion of macro
‘be32_to_cpu’
  299 |  r7 = be32_to_cpu(p[2]);
      |       ^~~~~~~~~~~
drivers/tty/ehv_bytechan.c:166:13: note: while referencing ‘data’
  166 | static void ehv_bc_udbg_putc(char c)
      |             ^~~~~~~~~~~~~~~~
In file included from include/linux/byteorder/big_endian.h:5,
                 from arch/powerpc/include/uapi/asm/byteorder.h:14,
                 from include/asm-generic/bitops/le.h:6,
                 from arch/powerpc/include/asm/bitops.h:250,
                 from include/linux/bitops.h:29,
                 from include/linux/kernel.h:12,
                 from include/asm-generic/bug.h:19,
                 from arch/powerpc/include/asm/bug.h:109,
                 from include/linux/bug.h:5,
                 from include/linux/mmdebug.h:5,
                 from include/linux/gfp.h:5,
                 from include/linux/slab.h:15,
                 from drivers/tty/ehv_bytechan.c:24:
arch/powerpc/include/asm/epapr_hcalls.h:300:20: warning: array subscript 3
is outside array bounds of ‘const char[1]’ [-Warray-bounds]
  300 |  r8 = be32_to_cpu(p[3]);
include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of
macro ‘__be32_to_cpu’
   40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
      |                                                   ^
arch/powerpc/include/asm/epapr_hcalls.h:300:7: note: in expansion of macro
‘be32_to_cpu’
  300 |  r8 = be32_to_cpu(p[3]);
      |       ^~~~~~~~~~~
drivers/tty/ehv_bytechan.c:166:13: note: while referencing ‘data’
  166 | static void ehv_bc_udbg_putc(char c)
      |             ^~~~~~~~~~~~~~~~

Signed-off-by: Stephen Rothwell <redacted>
---

Tested-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>

---
Best Regards, Laurentiu