Re: [PATCH] evh_bytechan: fix out of bounds accesses
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
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 25.02.2020 22:56, Stephen Rothwell wrote: Hi Laurentiu, On Tue, 25 Feb 2020 11:54:17 +0200 Laurentiu Tudor wrote: On 21.02.2020 01:57, Stephen Rothwell wrote: On Thu, 16 Jan 2020 11:37:14 +1100 Stephen Rothwell wrote: On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood wrote: On Thu, 2020-01-16 at 06:42 +1100, Stephen Rothwell wrote: On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi 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 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
Re: [PATCH] evh_bytechan: fix out of bounds accesses
Hi Laurentiu, On Tue, 25 Feb 2020 11:54:17 +0200 Laurentiu Tudor wrote: > > On 21.02.2020 01:57, Stephen Rothwell wrote: > > > > On Thu, 16 Jan 2020 11:37:14 +1100 Stephen Rothwell > > wrote: > >> > >> On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood wrote: > >>> > >>> On Thu, 2020-01-16 at 06:42 +1100, Stephen Rothwell wrote: > > On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi 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 So, whose tree should his go via? -- Cheers, Stephen Rothwell pgp8HKriNlqei.pgp Description: OpenPGP digital signature
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 21.02.2020 01:57, Stephen Rothwell wrote: Hi all, On Thu, 16 Jan 2020 11:37:14 +1100 Stephen Rothwell wrote: On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood 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 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 --- Best Regards, Laurentiu
Re: [PATCH] evh_bytechan: fix out of bounds accesses
Hi all, On Thu, 16 Jan 2020 11:37:14 +1100 Stephen Rothwell wrote: > > On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood 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 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? -- Cheers, Stephen Rothwell pgpEm6beMpIil.pgp Description: OpenPGP digital signature
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
Hi Scott, On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood 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 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. -- Cheers, Stephen Rothwell pgp_NDSCYduOm.pgp Description: OpenPGP digital signature
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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 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
Re: [PATCH] evh_bytechan: fix out of bounds accesses
Hi Timur, On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi 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? -- Cheers, Stephen Rothwell pgpWNHPvSOiXD.pgp Description: OpenPGP digital signature
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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?
RE: [PATCH] evh_bytechan: fix out of bounds accesses
> -Original Message- > From: Linuxppc-dev bounces+laurentiu.tudor=nxp@lists.ozlabs.org> On Behalf Of Stephen > Rothwell > Sent: Tuesday, January 14, 2020 8:32 AM > To: Timur Tabi > Cc: b08...@gmail.com; Greg Kroah-Hartman ; > Jiri Slaby ; York Sun ; PowerPC Mailing > List ; sw...@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 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 > 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 &g
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 14.01.2020 03:10, Michael Ellerman wrote: > Laurentiu Tudor 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
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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
Re: [PATCH] evh_bytechan: fix out of bounds accesses
Hi Timur, On Mon, 13 Jan 2020 19:10:11 -0600 Timur Tabi 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 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:
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
Laurentiu Tudor 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
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
Hi Timur, On Mon, 13 Jan 2020 10:03:18 -0600 Timur Tabi 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. -- Cheers, Stephen Rothwell pgp69n8DME1wI.pgp Description: OpenPGP digital signature
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On Thu, Jan 9, 2020 at 1:41 AM 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.
...
> +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([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.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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
Re: [PATCH] evh_bytechan: fix out of bounds accesses
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.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
Stephen Rothwell 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
> Cc: PowerPC Mailing List
> Signed-off-by: Stephen Rothwell
> ---
> 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([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,
> , );
> } 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, , s);
> + ret = local_ev_byte_channel_send(handle, , 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, , bc->buf +
> bc->tail);
> + ret = local_ev_byte_channel_send(bc->handle, , 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
