From: Stephen Rothwell <sfr@canb.auug.org.au>
To: Timur Tabi <timur@kernel.org>
Cc: b08248@gmail.com, Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jslaby@suse.com>, york sun <york.sun@nxp.com>,
PowerPC Mailing List <linuxppc-dev@lists.ozlabs.org>,
swood@redhat.com
Subject: Re: [PATCH] evh_bytechan: fix out of bounds accesses
Date: Tue, 14 Jan 2020 17:31:41 +1100 [thread overview]
Message-ID: <20200114173141.29564b25@canb.auug.org.au> (raw)
In-Reply-To: <6ec4bc30-0526-672c-4261-3ad2cf69dd94@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 13861 bytes --]
Hi Timur,
On Mon, 13 Jan 2020 19:10:11 -0600 Timur Tabi <timur@kernel.org> wrote:
>
> On 1/13/20 2:25 PM, Stephen Rothwell wrote:
> > 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
> > 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
> > 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 <sfr@canb.auug.org.au>
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 <sfr@canb.auug.org.au>
---
arch/powerpc/include/asm/epapr_hcalls.h | 42 ----------------------
drivers/tty/ehv_bytechan.c | 48 +++++++++++++++++++++++++
2 files changed, 48 insertions(+), 42 deletions(-)
diff --git a/arch/powerpc/include/asm/epapr_hcalls.h b/arch/powerpc/include/asm/epapr_hcalls.h
index d3a7e36f1402..75c5943c9f85 100644
--- a/arch/powerpc/include/asm/epapr_hcalls.h
+++ b/arch/powerpc/include/asm/epapr_hcalls.h
@@ -268,48 +268,6 @@ static inline unsigned int ev_int_eoi(unsigned int interrupt)
return r3;
}
-/**
- * ev_byte_channel_send - send characters to a byte stream
- * @handle: byte stream handle
- * @count: (input) num of chars to send, (output) num chars sent
- * @buffer: pointer to a 16-byte buffer
- *
- * @buffer must be at least 16 bytes long, because all 16 bytes will be
- * read from memory into registers, even if count < 16.
- *
- * Returns 0 for success, or an error code.
- */
-static inline unsigned int ev_byte_channel_send(unsigned int handle,
- unsigned int *count, const char buffer[EV_BYTE_CHANNEL_MAX_BYTES])
-{
- register uintptr_t r11 __asm__("r11");
- register uintptr_t r3 __asm__("r3");
- register uintptr_t r4 __asm__("r4");
- register uintptr_t r5 __asm__("r5");
- register uintptr_t r6 __asm__("r6");
- register uintptr_t r7 __asm__("r7");
- register uintptr_t r8 __asm__("r8");
- const uint32_t *p = (const uint32_t *) buffer;
-
- r11 = EV_HCALL_TOKEN(EV_BYTE_CHANNEL_SEND);
- r3 = handle;
- r4 = *count;
- r5 = be32_to_cpu(p[0]);
- r6 = be32_to_cpu(p[1]);
- r7 = be32_to_cpu(p[2]);
- r8 = be32_to_cpu(p[3]);
-
- asm volatile("bl epapr_hypercall_start"
- : "+r" (r11), "+r" (r3),
- "+r" (r4), "+r" (r5), "+r" (r6), "+r" (r7), "+r" (r8)
- : : EV_HCALL_CLOBBERS6
- );
-
- *count = r4;
-
- return r3;
-}
-
/**
* ev_byte_channel_receive - fetch characters from a byte channel
* @handle: byte channel handle
diff --git a/drivers/tty/ehv_bytechan.c b/drivers/tty/ehv_bytechan.c
index 769e0a5d1dfc..a5512745d0f9 100644
--- a/drivers/tty/ehv_bytechan.c
+++ b/drivers/tty/ehv_bytechan.c
@@ -136,6 +136,54 @@ static int find_console_handle(void)
return 1;
}
+/**
+ * ev_byte_channel_send - send characters to a byte stream
+ * @handle: byte stream handle
+ * @count: (input) num of chars to send, (output) num chars sent
+ * @bp: pointer to chars to send
+ *
+ * Returns 0 for success, or an error code.
+ */
+static unsigned int ev_byte_channel_send(unsigned int handle,
+ unsigned int *count, const char *bp)
+{
+ register uintptr_t r11 __asm__("r11");
+ register uintptr_t r3 __asm__("r3");
+ register uintptr_t r4 __asm__("r4");
+ register uintptr_t r5 __asm__("r5");
+ register uintptr_t r6 __asm__("r6");
+ register uintptr_t r7 __asm__("r7");
+ register uintptr_t r8 __asm__("r8");
+ const uint32_t *p;
+ char buffer[EV_BYTE_CHANNEL_MAX_BYTES];
+ unsigned int c = *count;
+
+ if (c < sizeof(buffer)) {
+ memcpy(buffer, bp, c);
+ memset(&buffer[c], 0, sizeof(buffer) - c);
+ p = (const uint32_t *)buffer;
+ } else {
+ p = (const uint32_t *)bp;
+ }
+ r11 = EV_HCALL_TOKEN(EV_BYTE_CHANNEL_SEND);
+ r3 = handle;
+ r4 = *count;
+ r5 = be32_to_cpu(p[0]);
+ r6 = be32_to_cpu(p[1]);
+ r7 = be32_to_cpu(p[2]);
+ r8 = be32_to_cpu(p[3]);
+
+ asm volatile("bl epapr_hypercall_start"
+ : "+r" (r11), "+r" (r3),
+ "+r" (r4), "+r" (r5), "+r" (r6), "+r" (r7), "+r" (r8)
+ : : EV_HCALL_CLOBBERS6
+ );
+
+ *count = r4;
+
+ return r3;
+}
+
/*************************** EARLY CONSOLE DRIVER ***************************/
#ifdef CONFIG_PPC_EARLY_DEBUG_EHV_BC
--
2.25.0.rc2
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2020-01-14 6:33 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-09 7:39 [PATCH] evh_bytechan: fix out of bounds accesses Stephen Rothwell
2020-01-13 12:26 ` Michael Ellerman
2020-01-13 13:48 ` Timur Tabi
2020-01-13 14:34 ` Laurentiu Tudor
2020-01-13 15:48 ` Timur Tabi
2020-01-14 1:10 ` Michael Ellerman
2020-01-14 9:18 ` Laurentiu Tudor
2020-01-14 11:01 ` Timur Tabi
2020-01-13 16:03 ` Timur Tabi
2020-01-13 20:25 ` Stephen Rothwell
2020-01-14 1:10 ` Timur Tabi
2020-01-14 1:13 ` Timur Tabi
2020-01-14 1:17 ` Scott Wood
2020-01-14 6:31 ` Stephen Rothwell [this message]
2020-01-15 12:33 ` Laurentiu Tudor
2020-01-15 13:25 ` Timur Tabi
2020-01-15 19:42 ` Stephen Rothwell
2020-01-15 20:01 ` Scott Wood
2020-01-16 0:37 ` Stephen Rothwell
2020-02-20 23:57 ` Stephen Rothwell
2020-02-25 9:54 ` Laurentiu Tudor
2020-02-25 20:56 ` Stephen Rothwell
2020-02-26 9:43 ` Laurentiu Tudor
2020-01-16 2:29 ` Timur Tabi
2020-01-14 8:29 ` Segher Boessenkool
2020-01-14 11:53 ` Timur Tabi
2020-01-14 12:24 ` Segher Boessenkool
2020-03-17 13:14 ` Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200114173141.29564b25@canb.auug.org.au \
--to=sfr@canb.auug.org.au \
--cc=b08248@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=jslaby@suse.com \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=swood@redhat.com \
--cc=timur@kernel.org \
--cc=york.sun@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.