2023-12-14 05:56:56

by Nicholas Miehlbradt

[permalink] [raw]
Subject: [PATCH 02/13] hvc: Fix use of uninitialized array in udbg_hvc_putc

All elements of bounce_buffer are eventually read and passed to the
hypervisor so it should probably be fully initialized.

Signed-off-by: Nicholas Miehlbradt <[email protected]>
---
drivers/tty/hvc/hvc_vio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/hvc/hvc_vio.c b/drivers/tty/hvc/hvc_vio.c
index 736b230f5ec0..1e88bfcdde20 100644
--- a/drivers/tty/hvc/hvc_vio.c
+++ b/drivers/tty/hvc/hvc_vio.c
@@ -227,7 +227,7 @@ static const struct hv_ops hvterm_hvsi_ops = {
static void udbg_hvc_putc(char c)
{
int count = -1;
- unsigned char bounce_buffer[16];
+ unsigned char bounce_buffer[16] = { 0 };

if (!hvterm_privs[0])
return;
--
2.40.1


2023-12-14 08:37:14

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH 02/13] hvc: Fix use of uninitialized array in udbg_hvc_putc



Le 14/12/2023 à 06:55, Nicholas Miehlbradt a écrit :
> All elements of bounce_buffer are eventually read and passed to the
> hypervisor so it should probably be fully initialized.

should or shall ?

>
> Signed-off-by: Nicholas Miehlbradt <[email protected]>

Should be a Fixed: tag ?

> ---
> drivers/tty/hvc/hvc_vio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/hvc/hvc_vio.c b/drivers/tty/hvc/hvc_vio.c
> index 736b230f5ec0..1e88bfcdde20 100644
> --- a/drivers/tty/hvc/hvc_vio.c
> +++ b/drivers/tty/hvc/hvc_vio.c
> @@ -227,7 +227,7 @@ static const struct hv_ops hvterm_hvsi_ops = {
> static void udbg_hvc_putc(char c)
> {
> int count = -1;
> - unsigned char bounce_buffer[16];
> + unsigned char bounce_buffer[16] = { 0 };

Why 16 while we have a count of 1 in the call to hvterm_raw_put_chars() ?

>
> if (!hvterm_privs[0])
> return;

2023-12-21 12:10:31

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH 02/13] hvc: Fix use of uninitialized array in udbg_hvc_putc

Christophe Leroy <[email protected]> writes:
> Le 14/12/2023 à 06:55, Nicholas Miehlbradt a écrit :
>> All elements of bounce_buffer are eventually read and passed to the
>> hypervisor so it should probably be fully initialized.
>
> should or shall ?
>
>>
>> Signed-off-by: Nicholas Miehlbradt <[email protected]>
>
> Should be a Fixed: tag ?
>
>> ---
>> drivers/tty/hvc/hvc_vio.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/hvc/hvc_vio.c b/drivers/tty/hvc/hvc_vio.c
>> index 736b230f5ec0..1e88bfcdde20 100644
>> --- a/drivers/tty/hvc/hvc_vio.c
>> +++ b/drivers/tty/hvc/hvc_vio.c
>> @@ -227,7 +227,7 @@ static const struct hv_ops hvterm_hvsi_ops = {
>> static void udbg_hvc_putc(char c)
>> {
>> int count = -1;
>> - unsigned char bounce_buffer[16];
>> + unsigned char bounce_buffer[16] = { 0 };
>
> Why 16 while we have a count of 1 in the call to hvterm_raw_put_chars() ?

Because hvterm_raw_put_chars() calls hvc_put_chars() which requires a 16
byte buffer, because it passes the buffer directly to firmware which
expects a 16 byte buffer.

It's a pretty horrible calling convention, but I guess it's to avoid
needing another bounce buffer inside hvc_put_chars().

We should probably do the change below, to at least document the
interface better.

cheers


diff --git a/arch/powerpc/include/asm/hvconsole.h b/arch/powerpc/include/asm/hvconsole.h
index ccb2034506f0..0ee7ed019e23 100644
--- a/arch/powerpc/include/asm/hvconsole.h
+++ b/arch/powerpc/include/asm/hvconsole.h
@@ -22,7 +22,7 @@
* parm is included to conform to put_chars() function pointer template
*/
extern int hvc_get_chars(uint32_t vtermno, char *buf, int count);
-extern int hvc_put_chars(uint32_t vtermno, const char *buf, int count);
+extern int hvc_put_chars(uint32_t vtermno, const char buf[16], int count);

/* Provided by HVC VIO */
void hvc_vio_init_early(void);
diff --git a/arch/powerpc/platforms/pseries/hvconsole.c b/arch/powerpc/platforms/pseries/hvconsole.c
index 1ac52963e08b..c40a82e49d59 100644
--- a/arch/powerpc/platforms/pseries/hvconsole.c
+++ b/arch/powerpc/platforms/pseries/hvconsole.c
@@ -52,7 +52,7 @@ EXPORT_SYMBOL(hvc_get_chars);
* firmware. Must be at least 16 bytes, even if count is less than 16.
* @count: Send this number of characters.
*/
-int hvc_put_chars(uint32_t vtermno, const char *buf, int count)
+int hvc_put_chars(uint32_t vtermno, const char buf[16], int count)
{
unsigned long *lbuf = (unsigned long *) buf;
long ret;
diff --git a/drivers/tty/hvc/hvc_vio.c b/drivers/tty/hvc/hvc_vio.c
index 736b230f5ec0..011b239a7e52 100644
--- a/drivers/tty/hvc/hvc_vio.c
+++ b/drivers/tty/hvc/hvc_vio.c
@@ -115,7 +115,7 @@ static int hvterm_raw_get_chars(uint32_t vtermno, char *buf, int count)
* you are sending fewer chars.
* @count: number of chars to send.
*/
-static int hvterm_raw_put_chars(uint32_t vtermno, const char *buf, int count)
+static int hvterm_raw_put_chars(uint32_t vtermno, const char buf[16], int count)
{
struct hvterm_priv *pv = hvterm_privs[vtermno];