| Message ID | 20200109183912.5fcb52aa@canb.auug.org.au (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3670664b5da555a2a481449b3baafff113b0ac35 |
| Headers | show |
| Series | evh_bytechan: fix out of bounds accesses | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (20862247a368dbb75d6e97d82345999adaacf3cc) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | fail | total: 1 errors, 1 warnings, 2 checks, 44 lines checked |
| snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
Stephen Rothwell <sfr@canb.auug.org.au> writes: > 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 make sure we actually pass a 16 byte array. > > There may be more elegant solutions to this, but the driver is quite > old and hasn't been updated in many years. ... > Fixes: dcd83aaff1c8 ("tty/powerpc: introduce the ePAPR embedded hypervisor byte channel driver") > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: PowerPC Mailing List <linuxppc-dev@lists.ozlabs.org> > Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au> > --- > drivers/tty/ehv_bytechan.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > I have only build tested this change so it would be good to get some > response from the PowerPC maintainers/developers. I've never heard of it, and I have no idea how to test it. It's not used by qemu, I guess there is/was a Freescale hypervisor that used it. But maybe it's time to remove it if it's not being maintained/used by anyone? cheers > diff --git a/drivers/tty/ehv_bytechan.c b/drivers/tty/ehv_bytechan.c > index 769e0a5d1dfc..546f80c49ae6 100644 > --- a/drivers/tty/ehv_bytechan.c > +++ b/drivers/tty/ehv_bytechan.c > @@ -136,6 +136,20 @@ static int find_console_handle(void) > return 1; > } > > +static unsigned int local_ev_byte_channel_send(unsigned int handle, > + unsigned int *count, const char *p) > +{ > + char buffer[EV_BYTE_CHANNEL_MAX_BYTES]; > + unsigned int c = *count; > + > + if (c < sizeof(buffer)) { > + memcpy(buffer, p, c); > + memset(&buffer[c], 0, sizeof(buffer) - c); > + p = buffer; > + } > + return ev_byte_channel_send(handle, count, p); > +} > + > /*************************** EARLY CONSOLE DRIVER ***************************/ > > #ifdef CONFIG_PPC_EARLY_DEBUG_EHV_BC > @@ -154,7 +168,7 @@ static void byte_channel_spin_send(const char data) > > do { > count = 1; > - ret = ev_byte_channel_send(CONFIG_PPC_EARLY_DEBUG_EHV_BC_HANDLE, > + ret = local_ev_byte_channel_send(CONFIG_PPC_EARLY_DEBUG_EHV_BC_HANDLE, > &count, &data); > } while (ret == EV_EAGAIN); > } > @@ -221,7 +235,7 @@ static int ehv_bc_console_byte_channel_send(unsigned int handle, const char *s, > while (count) { > len = min_t(unsigned int, count, EV_BYTE_CHANNEL_MAX_BYTES); > do { > - ret = ev_byte_channel_send(handle, &len, s); > + ret = local_ev_byte_channel_send(handle, &len, s); > } while (ret == EV_EAGAIN); > count -= len; > s += len; > @@ -401,7 +415,7 @@ static void ehv_bc_tx_dequeue(struct ehv_bc_data *bc) > CIRC_CNT_TO_END(bc->head, bc->tail, BUF_SIZE), > EV_BYTE_CHANNEL_MAX_BYTES); > > - ret = ev_byte_channel_send(bc->handle, &len, bc->buf + bc->tail); > + ret = local_ev_byte_channel_send(bc->handle, &len, bc->buf + bc->tail); > > /* 'len' is valid only if the return code is 0 or EV_EAGAIN */ > if (!ret || (ret == EV_EAGAIN)) > -- > 2.25.0.rc1 > > -- > Cheers, > Stephen Rothwell
On 1/13/20 6:26 AM, Michael Ellerman wrote: > I've never heard of it, and I have no idea how to test it. > > It's not used by qemu, I guess there is/was a Freescale hypervisor that > used it. Yes, there is/was a Freescale hypervisor that I and a few others worked on. I've added a couple people on CC that might be able to tell the current disposition of it. > But maybe it's time to remove it if it's not being maintained/used by > anyone? I wouldn't be completely opposed to that if there really are no more users. There really weren't any users even when I wrote the driver. I haven't had a chance to study the patch, but my first instinct is that there's got to be a better way to fix this than introducing a memcpy.
Hello, On 13.01.2020 15:48, Timur Tabi wrote: > On 1/13/20 6:26 AM, Michael Ellerman wrote: >> I've never heard of it, and I have no idea how to test it. >> >> It's not used by qemu, I guess there is/was a Freescale hypervisor that >> used it. > > Yes, there is/was a Freescale hypervisor that I and a few others worked > on. I've added a couple people on CC that might be able to tell the > current disposition of it. More info on this: it's opensource and it's published here [1]. We still claim to maintain it but there wasn't much activity past years, as one might notice. >> But maybe it's time to remove it if it's not being maintained/used by >> anyone? > > I wouldn't be completely opposed to that if there really are no more > users. There really weren't any users even when I wrote the driver. There are a few users that I know of, but I can't tell if that's enough to justify keeping the driver. [1] https://source.codeaurora.org/external/qoriq/qoriq-yocto-sdk/hypervisor/ --- Best Regards, Laurentiu
On 1/13/20 8:34 AM, Laurentiu Tudor wrote: > There are a few users that I know of, but I can't tell if that's enough > to justify keeping the driver. > > [1]https://source.codeaurora.org/external/qoriq/qoriq-yocto-sdk/hypervisor/ IIRC, the driver is the only reasonable way to get a serial console from a guest. So if there are users of the hypervisor, then I think there's a good chance at least one is using the byte channel driver.
On Thu, Jan 9, 2020 at 1:41 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote: > > 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 make sure we actually pass a 16 byte array. ... > +static unsigned int local_ev_byte_channel_send(unsigned int handle, > + unsigned int *count, const char *p) > +{ > + char buffer[EV_BYTE_CHANNEL_MAX_BYTES]; > + unsigned int c = *count; > + > + if (c < sizeof(buffer)) { > + memcpy(buffer, p, c); > + memset(&buffer[c], 0, sizeof(buffer) - c); > + p = buffer; > + } > + return ev_byte_channel_send(handle, count, p); > +} Why not simply correct the parameters of ev_byte_channel_send? static inline unsigned int ev_byte_channel_send(unsigned int handle, -unsigned int *count, const char buffer[EV_BYTE_CHANNEL_MAX_BYTES]) +unsigned int *count, const char *buffer) Back then, I probably thought I was just being clever with this code, but I realize now that it doesn't make sense to do the way I did.
Hi Timur, On Mon, 13 Jan 2020 10:03:18 -0600 Timur Tabi <timur@kernel.org> wrote: > > Why not simply correct the parameters of ev_byte_channel_send? > > static inline unsigned int ev_byte_channel_send(unsigned int handle, > -unsigned int *count, const char buffer[EV_BYTE_CHANNEL_MAX_BYTES]) > +unsigned int *count, const char *buffer) > > Back then, I probably thought I was just being clever with this code, > but I realize now that it doesn't make sense to do the way I did. 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.
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.
Laurentiu Tudor <laurentiu.tudor@nxp.com> writes: > Hello, > > On 13.01.2020 15:48, Timur Tabi wrote: >> On 1/13/20 6:26 AM, Michael Ellerman wrote: >>> I've never heard of it, and I have no idea how to test it. >>> >>> It's not used by qemu, I guess there is/was a Freescale hypervisor that >>> used it. >> >> Yes, there is/was a Freescale hypervisor that I and a few others worked >> on. I've added a couple people on CC that might be able to tell the >> current disposition of it. > > More info on this: it's opensource and it's published here [1]. We still > claim to maintain it but there wasn't much activity past years, as one > might notice. > >>> But maybe it's time to remove it if it's not being maintained/used by >>> anyone? >> >> I wouldn't be completely opposed to that if there really are no more >> users. There really weren't any users even when I wrote the driver. > > There are a few users that I know of, but I can't tell if that's enough > to justify keeping the driver. It is, I don't want to remove code that people are actually using, unless it's causing unsustainable maintenance burden. But this should be easy enough to get fixed. Could you or someone else at NXP volunteer to maintain this driver? That shouldn't involve much work, other than fixing small things like this warning. cheers
On 1/13/20 7:10 PM, Timur Tabi wrote: > > 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. Looks like ev_byte_channel_receive() has the same bug, but in reverse.
On Mon, 2020-01-13 at 19:13 -0600, Timur Tabi wrote: > On 1/13/20 7:10 PM, Timur Tabi wrote: > > 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. > > Looks like ev_byte_channel_receive() has the same bug, but in reverse. It only has one user, which always passes in a 16-byte buffer. -Scott
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
On Mon, Jan 13, 2020 at 07:10:11PM -0600, Timur Tabi wrote: > 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. You have no working lswx I suppose? :-) /me slowly backs away Segher
On 14.01.2020 03:10, Michael Ellerman wrote: > Laurentiu Tudor <laurentiu.tudor@nxp.com> writes: >> Hello, >> >> On 13.01.2020 15:48, Timur Tabi wrote: >>> On 1/13/20 6:26 AM, Michael Ellerman wrote: >>>> I've never heard of it, and I have no idea how to test it. >>>> >>>> It's not used by qemu, I guess there is/was a Freescale hypervisor that >>>> used it. >>> >>> Yes, there is/was a Freescale hypervisor that I and a few others worked >>> on. I've added a couple people on CC that might be able to tell the >>> current disposition of it. >> >> More info on this: it's opensource and it's published here [1]. We still >> claim to maintain it but there wasn't much activity past years, as one >> might notice. >> >>>> But maybe it's time to remove it if it's not being maintained/used by >>>> anyone? >>> >>> I wouldn't be completely opposed to that if there really are no more >>> users. There really weren't any users even when I wrote the driver. >> >> There are a few users that I know of, but I can't tell if that's enough >> to justify keeping the driver. > > It is, I don't want to remove code that people are actually using, > unless it's causing unsustainable maintenance burden. > > But this should be easy enough to get fixed. Right. I see that Stephen already came up with a proposal for a fix. > Could you or someone else at NXP volunteer to maintain this driver? That > shouldn't involve much work, other than fixing small things like this > warning. I can offer myself. I'll send a MAINTAINERS patch if nobody is against it. --- Best Regards, Laurentiu
On 1/14/20 3:18 AM, Laurentiu Tudor wrote:
> I can offer myself. I'll send a MAINTAINERS patch if nobody is against it.
Yes, please do. I'm always available if you have any questions on the code.
On 1/14/20 2:29 AM, Segher Boessenkool wrote:
> You have no working lswx I suppose?:-)
I don't know if the P4080 supports lswx, but it does, than that would be
an elegant way to fix this bug.
On Tue, Jan 14, 2020 at 05:53:41AM -0600, Timur Tabi wrote: > On 1/14/20 2:29 AM, Segher Boessenkool wrote: > >You have no working lswx I suppose?:-) > > I don't know if the P4080 supports lswx, but it does, than that would be > an elegant way to fix this bug. No e500 version supports it. Many other CPUs do not allow it in little- endian mode, or not in real mode, etc. Segher
> -----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 <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 > > 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> > --- Tested-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> --- Best Regards, Laurentiu
On 1/14/20 12:31 AM, Stephen Rothwell wrote: > +/** > + * 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) Well, now you've moved this into the .c file and it is no longer available to other callers. Anything wrong with keeping it in the .h file?
Hi Timur, On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi <timur@kernel.org> wrote: > > On 1/14/20 12:31 AM, Stephen Rothwell wrote: > > +/** > > + * 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) > > Well, now you've moved this into the .c file and it is no longer > available to other callers. Anything wrong with keeping it in the .h file? There are currently no other callers - are there likely to be in the future? Even if there are, is it time critical enough that it needs to be inlined everywhere?
On Thu, 2020-01-16 at 06:42 +1100, Stephen Rothwell wrote: > Hi Timur, > > On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi <timur@kernel.org> wrote: > > On 1/14/20 12:31 AM, Stephen Rothwell wrote: > > > +/** > > > + * 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) > > > > Well, now you've moved this into the .c file and it is no longer > > available to other callers. Anything wrong with keeping it in the .h > > file? > > There are currently no other callers - are there likely to be in the > future? Even if there are, is it time critical enough that it needs to > be inlined everywhere? It's not performance critical and there aren't likely to be other users -- just a matter of what's cleaner. FWIW I'd rather see the original patch, that keeps the raw asm hcall stuff as simple wrappers in one place. -Scott
Hi Scott, On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood <swood@redhat.com> wrote: > > On Thu, 2020-01-16 at 06:42 +1100, Stephen Rothwell wrote: > > Hi Timur, > > > > On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi <timur@kernel.org> wrote: > > > On 1/14/20 12:31 AM, Stephen Rothwell wrote: > > > > +/** > > > > + * 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) > > > > > > Well, now you've moved this into the .c file and it is no longer > > > available to other callers. Anything wrong with keeping it in the .h > > > file? > > > > There are currently no other callers - are there likely to be in the > > future? Even if there are, is it time critical enough that it needs to > > be inlined everywhere? > > It's not performance critical and there aren't likely to be other users -- > just a matter of what's cleaner. FWIW I'd rather see the original patch, > that keeps the raw asm hcall stuff as simple wrappers in one place. And I don't mind either way :-) I just want to get rid of the warnings.
On 1/15/20 2:01 PM, Scott Wood wrote: > FWIW I'd rather see the original patch, > that keeps the raw asm hcall stuff as simple wrappers in one place. I can live with that.
Hi all, On Thu, 16 Jan 2020 11:37:14 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote: > > On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood <swood@redhat.com> wrote: > > > > On Thu, 2020-01-16 at 06:42 +1100, Stephen Rothwell wrote: > > > Hi Timur, > > > > > > On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi <timur@kernel.org> wrote: > > > > On 1/14/20 12:31 AM, Stephen Rothwell wrote: > > > > > +/** > > > > > + * 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) > > > > > > > > Well, now you've moved this into the .c file and it is no longer > > > > available to other callers. Anything wrong with keeping it in the .h > > > > file? > > > > > > There are currently no other callers - are there likely to be in the > > > future? Even if there are, is it time critical enough that it needs to > > > be inlined everywhere? > > > > It's not performance critical and there aren't likely to be other users -- > > just a matter of what's cleaner. FWIW I'd rather see the original patch, > > that keeps the raw asm hcall stuff as simple wrappers in one place. > > And I don't mind either way :-) > > I just want to get rid of the warnings. Any progress with this?
On 21.02.2020 01:57, Stephen Rothwell wrote: > Hi all, > > On Thu, 16 Jan 2020 11:37:14 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote: >> >> On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood <swood@redhat.com> wrote: >>> >>> On Thu, 2020-01-16 at 06:42 +1100, Stephen Rothwell wrote: >>>> Hi Timur, >>>> >>>> On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi <timur@kernel.org> wrote: >>>>> On 1/14/20 12:31 AM, Stephen Rothwell wrote: >>>>>> +/** >>>>>> + * 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) >>>>> >>>>> Well, now you've moved this into the .c file and it is no longer >>>>> available to other callers. Anything wrong with keeping it in the .h >>>>> file? >>>> >>>> There are currently no other callers - are there likely to be in the >>>> future? Even if there are, is it time critical enough that it needs to >>>> be inlined everywhere? >>> >>> It's not performance critical and there aren't likely to be other users -- >>> just a matter of what's cleaner. FWIW I'd rather see the original patch, >>> that keeps the raw asm hcall stuff as simple wrappers in one place. >> >> And I don't mind either way :-) >> >> I just want to get rid of the warnings. > > Any progress with this? > I think that the consensus was to pick up the original patch that is, this one: https://patchwork.ozlabs.org/patch/1220186/ I've tested it too, so please feel free to add a: Tested-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> --- Best Regards, Laurentiu
Hi Laurentiu, On Tue, 25 Feb 2020 11:54:17 +0200 Laurentiu Tudor <laurentiu.tudor@nxp.com> wrote: > > On 21.02.2020 01:57, Stephen Rothwell wrote: > > > > On Thu, 16 Jan 2020 11:37:14 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote: > >> > >> On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood <swood@redhat.com> wrote: > >>> > >>> On Thu, 2020-01-16 at 06:42 +1100, Stephen Rothwell wrote: > >>>> > >>>> On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi <timur@kernel.org> wrote: > >>>>> On 1/14/20 12:31 AM, Stephen Rothwell wrote: > >>>>>> +/** > >>>>>> + * 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) > >>>>> > >>>>> Well, now you've moved this into the .c file and it is no longer > >>>>> available to other callers. Anything wrong with keeping it in the .h > >>>>> file? > >>>> > >>>> There are currently no other callers - are there likely to be in the > >>>> future? Even if there are, is it time critical enough that it needs to > >>>> be inlined everywhere? > >>> > >>> It's not performance critical and there aren't likely to be other users -- > >>> just a matter of what's cleaner. FWIW I'd rather see the original patch, > >>> that keeps the raw asm hcall stuff as simple wrappers in one place. > >> > >> And I don't mind either way :-) > >> > >> I just want to get rid of the warnings. > > > > Any progress with this? > > I think that the consensus was to pick up the original patch that is, > this one: https://patchwork.ozlabs.org/patch/1220186/ > > I've tested it too, so please feel free to add a: > > Tested-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> So, whose tree should his go via?
On 25.02.2020 22:56, Stephen Rothwell wrote: > Hi Laurentiu, > > On Tue, 25 Feb 2020 11:54:17 +0200 Laurentiu Tudor <laurentiu.tudor@nxp.com> wrote: >> >> On 21.02.2020 01:57, Stephen Rothwell wrote: >>> >>> On Thu, 16 Jan 2020 11:37:14 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote: >>>> >>>> On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood <swood@redhat.com> wrote: >>>>> >>>>> On Thu, 2020-01-16 at 06:42 +1100, Stephen Rothwell wrote: >>>>>> >>>>>> On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi <timur@kernel.org> wrote: >>>>>>> On 1/14/20 12:31 AM, Stephen Rothwell wrote: >>>>>>>> +/** >>>>>>>> + * 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) >>>>>>> >>>>>>> Well, now you've moved this into the .c file and it is no longer >>>>>>> available to other callers. Anything wrong with keeping it in the .h >>>>>>> file? >>>>>> >>>>>> There are currently no other callers - are there likely to be in the >>>>>> future? Even if there are, is it time critical enough that it needs to >>>>>> be inlined everywhere? >>>>> >>>>> It's not performance critical and there aren't likely to be other users -- >>>>> just a matter of what's cleaner. FWIW I'd rather see the original patch, >>>>> that keeps the raw asm hcall stuff as simple wrappers in one place. >>>> >>>> And I don't mind either way :-) >>>> >>>> I just want to get rid of the warnings. >>> >>> Any progress with this? >> >> I think that the consensus was to pick up the original patch that is, >> this one: https://patchwork.ozlabs.org/patch/1220186/ >> >> I've tested it too, so please feel free to add a: >> >> Tested-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > So, whose tree should his go via? > Maybe Scott or Michael can help us here. And while at it maybe, take a look at this patch [1] too. [1] https://patchwork.ozlabs.org/patch/1227780/ --- Best Regards, Laurentiu
On Thu, 2020-01-09 at 07:39:12 UTC, Stephen Rothwell wrote: > 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 make sure we actually pass a 16 byte array. > > There may be more elegant solutions to this, but the driver is quite > old and hasn't been updated in many years. > > The warnings (from a powerpc allyesconfig build) 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 =E2=80=98ehv_bc_udbg_putc=E2=80=99: > arch/powerpc/include/asm/epapr_hcalls.h:298:20: warning: array subscript 1 = > is outside array bounds of =E2=80=98const char[1]=E2=80=99 [-Warray-bounds] > 298 | r6 =3D be32_to_cpu(p[1]); > include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of mac= > ro =E2=80=98__be32_to_cpu=E2=80=99 > 40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) > | ^ > arch/powerpc/include/asm/epapr_hcalls.h:298:7: note: in expansion of macro = > =E2=80=98be32_to_cpu=E2=80=99 > 298 | r6 =3D be32_to_cpu(p[1]); > | ^~~~~~~~~~~ > drivers/tty/ehv_bytechan.c:166:13: note: while referencing =E2=80=98data=E2= > =80=99 > 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 =E2=80=98const char[1]=E2=80=99 [-Warray-bounds] > 299 | r7 =3D be32_to_cpu(p[2]); > include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of mac= > ro =E2=80=98__be32_to_cpu=E2=80=99 > 40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) > | ^ > arch/powerpc/include/asm/epapr_hcalls.h:299:7: note: in expansion of macro = > =E2=80=98be32_to_cpu=E2=80=99 > 299 | r7 =3D be32_to_cpu(p[2]); > | ^~~~~~~~~~~ > drivers/tty/ehv_bytechan.c:166:13: note: while referencing =E2=80=98data=E2= > =80=99 > 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 =E2=80=98const char[1]=E2=80=99 [-Warray-bounds] > 300 | r8 =3D be32_to_cpu(p[3]); > include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of mac= > ro =E2=80=98__be32_to_cpu=E2=80=99 > 40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) > | ^ > arch/powerpc/include/asm/epapr_hcalls.h:300:7: note: in expansion of macro = > =E2=80=98be32_to_cpu=E2=80=99 > 300 | r8 =3D be32_to_cpu(p[3]); > | ^~~~~~~~~~~ > drivers/tty/ehv_bytechan.c:166:13: note: while referencing =E2=80=98data=E2= > =80=99 > 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 =E2=80=98const char[1]=E2=80=99 [-Warray-bounds] > 298 | r6 =3D be32_to_cpu(p[1]); > include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of mac= > ro =E2=80=98__be32_to_cpu=E2=80=99 > 40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) > | ^ > arch/powerpc/include/asm/epapr_hcalls.h:298:7: note: in expansion of macro = > =E2=80=98be32_to_cpu=E2=80=99 > 298 | r6 =3D be32_to_cpu(p[1]); > | ^~~~~~~~~~~ > drivers/tty/ehv_bytechan.c:166:13: note: while referencing =E2=80=98data=E2= > =80=99 > 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 =E2=80=98const char[1]=E2=80=99 [-Warray-bounds] > 299 | r7 =3D be32_to_cpu(p[2]); > include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of mac= > ro =E2=80=98__be32_to_cpu=E2=80=99 > 40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) > | ^ > arch/powerpc/include/asm/epapr_hcalls.h:299:7: note: in expansion of macro = > =E2=80=98be32_to_cpu=E2=80=99 > 299 | r7 =3D be32_to_cpu(p[2]); > | ^~~~~~~~~~~ > drivers/tty/ehv_bytechan.c:166:13: note: while referencing =E2=80=98data=E2= > =80=99 > 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 =E2=80=98const char[1]=E2=80=99 [-Warray-bounds] > 300 | r8 =3D be32_to_cpu(p[3]); > include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of mac= > ro =E2=80=98__be32_to_cpu=E2=80=99 > 40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) > | ^ > arch/powerpc/include/asm/epapr_hcalls.h:300:7: note: in expansion of macro = > =E2=80=98be32_to_cpu=E2=80=99 > 300 | r8 =3D be32_to_cpu(p[3]); > | ^~~~~~~~~~~ > drivers/tty/ehv_bytechan.c:166:13: note: while referencing =E2=80=98data=E2= > =80=99 > 166 | static void ehv_bc_udbg_putc(char c) > | ^~~~~~~~~~~~~~~~ > > Fixes: dcd83aaff1c8 ("tty/powerpc: introduce the ePAPR embedded hypervisor = > byte channel driver") > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: PowerPC Mailing List <linuxppc-dev@lists.ozlabs.org> > Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/3670664b5da555a2a481449b3baafff113b0ac35 cheers
diff --git a/drivers/tty/ehv_bytechan.c b/drivers/tty/ehv_bytechan.c index 769e0a5d1dfc..546f80c49ae6 100644 --- a/drivers/tty/ehv_bytechan.c +++ b/drivers/tty/ehv_bytechan.c @@ -136,6 +136,20 @@ static int find_console_handle(void) return 1; } +static unsigned int local_ev_byte_channel_send(unsigned int handle, + unsigned int *count, const char *p) +{ + char buffer[EV_BYTE_CHANNEL_MAX_BYTES]; + unsigned int c = *count; + + if (c < sizeof(buffer)) { + memcpy(buffer, p, c); + memset(&buffer[c], 0, sizeof(buffer) - c); + p = buffer; + } + return ev_byte_channel_send(handle, count, p); +} + /*************************** EARLY CONSOLE DRIVER ***************************/ #ifdef CONFIG_PPC_EARLY_DEBUG_EHV_BC @@ -154,7 +168,7 @@ static void byte_channel_spin_send(const char data) do { count = 1; - ret = ev_byte_channel_send(CONFIG_PPC_EARLY_DEBUG_EHV_BC_HANDLE, + ret = local_ev_byte_channel_send(CONFIG_PPC_EARLY_DEBUG_EHV_BC_HANDLE, &count, &data); } while (ret == EV_EAGAIN); } @@ -221,7 +235,7 @@ static int ehv_bc_console_byte_channel_send(unsigned int handle, const char *s, while (count) { len = min_t(unsigned int, count, EV_BYTE_CHANNEL_MAX_BYTES); do { - ret = ev_byte_channel_send(handle, &len, s); + ret = local_ev_byte_channel_send(handle, &len, s); } while (ret == EV_EAGAIN); count -= len; s += len; @@ -401,7 +415,7 @@ static void ehv_bc_tx_dequeue(struct ehv_bc_data *bc) CIRC_CNT_TO_END(bc->head, bc->tail, BUF_SIZE), EV_BYTE_CHANNEL_MAX_BYTES); - ret = ev_byte_channel_send(bc->handle, &len, bc->buf + bc->tail); + ret = local_ev_byte_channel_send(bc->handle, &len, bc->buf + bc->tail); /* 'len' is valid only if the return code is 0 or EV_EAGAIN */ if (!ret || (ret == EV_EAGAIN))
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 make sure we actually pass a 16 byte array. There may be more elegant solutions to this, but the driver is quite old and hasn't been updated in many years. The warnings (from a powerpc allyesconfig build) 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) | ^~~~~~~~~~~~~~~~ Fixes: dcd83aaff1c8 ("tty/powerpc: introduce the ePAPR embedded hypervisor byte channel driver") Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: PowerPC Mailing List <linuxppc-dev@lists.ozlabs.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au> --- drivers/tty/ehv_bytechan.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) I have only build tested this change so it would be good to get some response from the PowerPC maintainers/developers.