Hi Timur,
On Mon, 13 Jan 2020 19:10:11 -0600 Timur Tabi [off-list ref] wrote:
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
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 <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]);131 more lines
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 handlediff --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