mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH] console: fix out-of-bounds read in dputc(/dev/*, ...)
@ 2019-07-30 15:48 Ahmad Fatoum
  2019-07-30 15:53 ` Ahmad Fatoum
  2019-07-31  7:05 ` Bastian Krause
  0 siblings, 2 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2019-07-30 15:48 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum, Bastian Krause

Trying to output a single character via
	echo -a /dev/serial0-1
currently results in garbage output after the newline, because console.c's
fops_write discards the buffer length and passes the buffer to
(struct cdev)::puts which only handles NUL-terminated strings.

Fix this by amending (struct cdev)::puts with a new nbytes parameter,
which is correctly propagated.

Fixes: 1233570798 ("console: expose consoles in devfs")
Cc: Bastian Krause <bst@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/console.c            | 12 ++++++------
 common/ratp/ratp.c          | 12 +++++-------
 drivers/serial/efi-stdio.c  |  5 +++--
 drivers/serial/serial_efi.c |  5 +++--
 fs/pstore/platform.c        |  7 ++++---
 include/console.h           |  2 +-
 6 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/common/console.c b/common/console.c
index 47ccf2e54de0..59218ea4e22f 100644
--- a/common/console.c
+++ b/common/console.c
@@ -253,17 +253,17 @@ static void console_set_stdoutpath(struct console_device *cdev)
 	free(str);
 }
 
-static int __console_puts(struct console_device *cdev, const char *s)
+static int __console_puts(struct console_device *cdev, const char *s,
+			  size_t nbytes)
 {
-	int n = 0;
+	int n;
 
-	while (*s) {
+	for (n = 0; n < nbytes; n++) {
 		if (*s == '\n') {
 			cdev->putc(cdev, '\r');
 			n++;
 		}
 		cdev->putc(cdev, *s);
-		n++;
 		s++;
 	}
 	return n;
@@ -298,7 +298,7 @@ static ssize_t fops_write(struct cdev* dev, const void* buf, size_t count,
 {
 	struct console_device *priv = dev->priv;
 
-	priv->puts(priv, buf);
+	priv->puts(priv, buf, count);
 
 	return 0;
 }
@@ -545,7 +545,7 @@ int console_puts(unsigned int ch, const char *str)
 	if (initialized == CONSOLE_INIT_FULL) {
 		for_each_console(cdev) {
 			if (cdev->f_active & ch) {
-				n = cdev->puts(cdev, str);
+				n = cdev->puts(cdev, str, strlen(str));
 			}
 		}
 		return n;
diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
index 9aea1786d684..e84ad221672a 100644
--- a/common/ratp/ratp.c
+++ b/common/ratp/ratp.c
@@ -259,19 +259,17 @@ static int ratp_console_tstc(struct console_device *cdev)
 	return kfifo_len(ctx->console_recv_fifo) ? 1 : 0;
 }
 
-static int ratp_console_puts(struct console_device *cdev, const char *s)
+static int ratp_console_puts(struct console_device *cdev, const char *s,
+			     size_t nbytes)
 {
 	struct ratp_ctx *ctx = container_of(cdev, struct ratp_ctx, ratp_console);
-	int len = 0;
-
-	len = strlen(s);
 
 	if (ratp_busy(&ctx->ratp))
-		return len;
+		return nbytes;
 
-	kfifo_put(ctx->console_transmit_fifo, s, len);
+	kfifo_put(ctx->console_transmit_fifo, s, nbytes);
 
-	return len;
+	return nbytes;
 }
 
 static void ratp_console_putc(struct console_device *cdev, char c)
diff --git a/drivers/serial/efi-stdio.c b/drivers/serial/efi-stdio.c
index 0703f727e766..2ca89fa4f861 100644
--- a/drivers/serial/efi-stdio.c
+++ b/drivers/serial/efi-stdio.c
@@ -243,12 +243,13 @@ static int efi_process_key(struct efi_console_priv *priv, const char *inp)
 	return 1;
 }
 
-static int efi_console_puts(struct console_device *cdev, const char *s)
+static int efi_console_puts(struct console_device *cdev, const char *s,
+			    size_t nbytes)
 {
 	struct efi_console_priv *priv = to_efi(cdev);
 	int n = 0;
 
-	while (*s) {
+	while (nbytes--) {
 		if (*s == 27) {
 			priv->efi_console_buffer[n] = 0;
 			priv->out->output_string(priv->out,
diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c
index f0a2b22c2bc2..667d51f622ec 100644
--- a/drivers/serial/serial_efi.c
+++ b/drivers/serial/serial_efi.c
@@ -130,13 +130,14 @@ static void efi_serial_putc(struct console_device *cdev, char c)
 	serial->write(serial, &buffersize, &c);
 }
 
-static int efi_serial_puts(struct console_device *cdev, const char *s)
+static int efi_serial_puts(struct console_device *cdev, const char *s,
+			   size_t nbytes)
 {
 	struct efi_serial_port *uart = to_efi_serial_port(cdev);
 	struct efi_serial_io_protocol *serial = uart->serial;
 	uint32_t control;
 	efi_status_t efiret;
-	unsigned long buffersize = strlen(s) * sizeof(char);
+	unsigned long buffersize = nbytes;
 
 	do {
 		efiret = serial->getcontrol(serial, &control);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 755363c30999..601bfca6b025 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -67,10 +67,11 @@ static void pstore_console_write(const char *s, unsigned c)
 	}
 }
 
-static int pstore_console_puts(struct console_device *cdev, const char *s)
+static int pstore_console_puts(struct console_device *cdev, const char *s,
+			       size_t nbytes)
 {
-	pstore_console_write(s, strlen(s));
-	return strlen(s);
+	pstore_console_write(s, nbytes);
+	return nbytes;
 }
 
 static void pstore_console_putc(struct console_device *cdev, char c)
diff --git a/include/console.h b/include/console.h
index 673921331db7..1d86a584a7f2 100644
--- a/include/console.h
+++ b/include/console.h
@@ -42,7 +42,7 @@ struct console_device {
 
 	int (*tstc)(struct console_device *cdev);
 	void (*putc)(struct console_device *cdev, char c);
-	int (*puts)(struct console_device *cdev, const char *s);
+	int (*puts)(struct console_device *cdev, const char *s, size_t nbytes);
 	int  (*getc)(struct console_device *cdev);
 	int (*setbrg)(struct console_device *cdev, int baudrate);
 	void (*flush)(struct console_device *cdev);
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] console: fix out-of-bounds read in dputc(/dev/*, ...)
  2019-07-30 15:48 [PATCH] console: fix out-of-bounds read in dputc(/dev/*, ...) Ahmad Fatoum
@ 2019-07-30 15:53 ` Ahmad Fatoum
  2019-07-31 10:23   ` Ahmad Fatoum
  2019-07-31  7:05 ` Bastian Krause
  1 sibling, 1 reply; 5+ messages in thread
From: Ahmad Fatoum @ 2019-07-30 15:53 UTC (permalink / raw)
  To: barebox; +Cc: Bastian Krause

On 30/7/19 17:48, Ahmad Fatoum wrote:
> Trying to output a single character via
> 	echo -a /dev/serial0-1
> currently results in garbage output after the newline, because console.c's
> fops_write discards the buffer length and passes the buffer to
> (struct cdev)::puts which only handles NUL-terminated strings.
> 
> Fix this by amending (struct cdev)::puts with a new nbytes parameter,
> which is correctly propagated.
> 
> Fixes: 1233570798 ("console: expose consoles in devfs")
> Cc: Bastian Krause <bst@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/console.c            | 12 ++++++------
>  common/ratp/ratp.c          | 12 +++++-------
>  drivers/serial/efi-stdio.c  |  5 +++--
>  drivers/serial/serial_efi.c |  5 +++--
>  fs/pstore/platform.c        |  7 ++++---
>  include/console.h           |  2 +-
>  6 files changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/common/console.c b/common/console.c
> index 47ccf2e54de0..59218ea4e22f 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -253,17 +253,17 @@ static void console_set_stdoutpath(struct console_device *cdev)
>  	free(str);
>  }
>  
> -static int __console_puts(struct console_device *cdev, const char *s)
> +static int __console_puts(struct console_device *cdev, const char *s,
> +			  size_t nbytes)
>  {
> -	int n = 0;
> +	int n;
>  
> -	while (*s) {
> +	for (n = 0; n < nbytes; n++) {
>  		if (*s == '\n') {
>  			cdev->putc(cdev, '\r');
>  			n++;
>  		}
>  		cdev->putc(cdev, *s);
> -		n++;
>  		s++;
>  	}
>  	return n;
> @@ -298,7 +298,7 @@ static ssize_t fops_write(struct cdev* dev, const void* buf, size_t count,
>  {
>  	struct console_device *priv = dev->priv;
>  
> -	priv->puts(priv, buf);
> +	priv->puts(priv, buf, count);
>  
>  	return 0;
>  }
> @@ -545,7 +545,7 @@ int console_puts(unsigned int ch, const char *str)
>  	if (initialized == CONSOLE_INIT_FULL) {
>  		for_each_console(cdev) {
>  			if (cdev->f_active & ch) {
> -				n = cdev->puts(cdev, str);
> +				n = cdev->puts(cdev, str, strlen(str));
>  			}
>  		}
>  		return n;
> diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
> index 9aea1786d684..e84ad221672a 100644
> --- a/common/ratp/ratp.c
> +++ b/common/ratp/ratp.c
> @@ -259,19 +259,17 @@ static int ratp_console_tstc(struct console_device *cdev)
>  	return kfifo_len(ctx->console_recv_fifo) ? 1 : 0;
>  }
>  
> -static int ratp_console_puts(struct console_device *cdev, const char *s)
> +static int ratp_console_puts(struct console_device *cdev, const char *s,
> +			     size_t nbytes)
>  {
>  	struct ratp_ctx *ctx = container_of(cdev, struct ratp_ctx, ratp_console);
> -	int len = 0;
> -
> -	len = strlen(s);
>  
>  	if (ratp_busy(&ctx->ratp))
> -		return len;
> +		return nbytes;

This didn't look right before. Should this be a return 0 instead?
Sascha?

>  
> -	kfifo_put(ctx->console_transmit_fifo, s, len);
> +	kfifo_put(ctx->console_transmit_fifo, s, nbytes);
>  
> -	return len;
> +	return nbytes;
>  }
>  
>  static void ratp_console_putc(struct console_device *cdev, char c)
> diff --git a/drivers/serial/efi-stdio.c b/drivers/serial/efi-stdio.c
> index 0703f727e766..2ca89fa4f861 100644
> --- a/drivers/serial/efi-stdio.c
> +++ b/drivers/serial/efi-stdio.c
> @@ -243,12 +243,13 @@ static int efi_process_key(struct efi_console_priv *priv, const char *inp)
>  	return 1;
>  }
>  
> -static int efi_console_puts(struct console_device *cdev, const char *s)
> +static int efi_console_puts(struct console_device *cdev, const char *s,
> +			    size_t nbytes)
>  {
>  	struct efi_console_priv *priv = to_efi(cdev);
>  	int n = 0;
>  
> -	while (*s) {
> +	while (nbytes--) {
>  		if (*s == 27) {
>  			priv->efi_console_buffer[n] = 0;
>  			priv->out->output_string(priv->out,
> diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c
> index f0a2b22c2bc2..667d51f622ec 100644
> --- a/drivers/serial/serial_efi.c
> +++ b/drivers/serial/serial_efi.c
> @@ -130,13 +130,14 @@ static void efi_serial_putc(struct console_device *cdev, char c)
>  	serial->write(serial, &buffersize, &c);
>  }
>  
> -static int efi_serial_puts(struct console_device *cdev, const char *s)
> +static int efi_serial_puts(struct console_device *cdev, const char *s,
> +			   size_t nbytes)
>  {
>  	struct efi_serial_port *uart = to_efi_serial_port(cdev);
>  	struct efi_serial_io_protocol *serial = uart->serial;
>  	uint32_t control;
>  	efi_status_t efiret;
> -	unsigned long buffersize = strlen(s) * sizeof(char);
> +	unsigned long buffersize = nbytes;
>  
>  	do {
>  		efiret = serial->getcontrol(serial, &control);
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 755363c30999..601bfca6b025 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -67,10 +67,11 @@ static void pstore_console_write(const char *s, unsigned c)
>  	}
>  }
>  
> -static int pstore_console_puts(struct console_device *cdev, const char *s)
> +static int pstore_console_puts(struct console_device *cdev, const char *s,
> +			       size_t nbytes)
>  {
> -	pstore_console_write(s, strlen(s));
> -	return strlen(s);
> +	pstore_console_write(s, nbytes);
> +	return nbytes;
>  }
>  
>  static void pstore_console_putc(struct console_device *cdev, char c)
> diff --git a/include/console.h b/include/console.h
> index 673921331db7..1d86a584a7f2 100644
> --- a/include/console.h
> +++ b/include/console.h
> @@ -42,7 +42,7 @@ struct console_device {
>  
>  	int (*tstc)(struct console_device *cdev);
>  	void (*putc)(struct console_device *cdev, char c);
> -	int (*puts)(struct console_device *cdev, const char *s);
> +	int (*puts)(struct console_device *cdev, const char *s, size_t nbytes);

Thoughts on renaming this to something else? Apparently non-matching function
pointers are just a warning with gcc 8.2.1 (OSELAS.Toolchain-2018.12.0).
I'd have preferred non-mainline serial drivers to completely fail to compile.

>  	int  (*getc)(struct console_device *cdev);
>  	int (*setbrg)(struct console_device *cdev, int baudrate);
>  	void (*flush)(struct console_device *cdev);
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] console: fix out-of-bounds read in dputc(/dev/*, ...)
  2019-07-30 15:48 [PATCH] console: fix out-of-bounds read in dputc(/dev/*, ...) Ahmad Fatoum
  2019-07-30 15:53 ` Ahmad Fatoum
@ 2019-07-31  7:05 ` Bastian Krause
  2019-07-31  8:24   ` Ahmad Fatoum
  1 sibling, 1 reply; 5+ messages in thread
From: Bastian Krause @ 2019-07-31  7:05 UTC (permalink / raw)
  To: Ahmad Fatoum, barebox

On 7/30/19 5:48 PM, Ahmad Fatoum wrote:
> Trying to output a single character via
> 	echo -a /dev/serial0-1
> currently results in garbage output after the newline, because console.c's
> fops_write discards the buffer length and passes the buffer to
> (struct cdev)::puts which only handles NUL-terminated strings.
> 
> Fix this by amending (struct cdev)::puts with a new nbytes parameter,
> which is correctly propagated.
> 
> Fixes: 1233570798 ("console: expose consoles in devfs")

The is the wrong commit id, I guess you mean b4f55fcf35.

> Cc: Bastian Krause <bst@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/console.c            | 12 ++++++------
>  common/ratp/ratp.c          | 12 +++++-------
>  drivers/serial/efi-stdio.c  |  5 +++--
>  drivers/serial/serial_efi.c |  5 +++--
>  fs/pstore/platform.c        |  7 ++++---
>  include/console.h           |  2 +-
>  6 files changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/common/console.c b/common/console.c
> index 47ccf2e54de0..59218ea4e22f 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -253,17 +253,17 @@ static void console_set_stdoutpath(struct console_device *cdev)
>  	free(str);
>  }
>  
> -static int __console_puts(struct console_device *cdev, const char *s)
> +static int __console_puts(struct console_device *cdev, const char *s,
> +			  size_t nbytes)
>  {
> -	int n = 0;
> +	int n;
>  
> -	while (*s) {
> +	for (n = 0; n < nbytes; n++) {
>  		if (*s == '\n') {
>  			cdev->putc(cdev, '\r');
>  			n++;
>  		}
>  		cdev->putc(cdev, *s);
> -		n++;
>  		s++;
>  	}
>  	return n;
> @@ -298,7 +298,7 @@ static ssize_t fops_write(struct cdev* dev, const void* buf, size_t count,
>  {
>  	struct console_device *priv = dev->priv;
>  
> -	priv->puts(priv, buf);
> +	priv->puts(priv, buf, count);
>  
>  	return 0;
>  }
> @@ -545,7 +545,7 @@ int console_puts(unsigned int ch, const char *str)
>  	if (initialized == CONSOLE_INIT_FULL) {
>  		for_each_console(cdev) {
>  			if (cdev->f_active & ch) {
> -				n = cdev->puts(cdev, str);
> +				n = cdev->puts(cdev, str, strlen(str));
>  			}
>  		}
>  		return n;
> diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
> index 9aea1786d684..e84ad221672a 100644
> --- a/common/ratp/ratp.c
> +++ b/common/ratp/ratp.c
> @@ -259,19 +259,17 @@ static int ratp_console_tstc(struct console_device *cdev)
>  	return kfifo_len(ctx->console_recv_fifo) ? 1 : 0;
>  }
>  
> -static int ratp_console_puts(struct console_device *cdev, const char *s)
> +static int ratp_console_puts(struct console_device *cdev, const char *s,
> +			     size_t nbytes)
>  {
>  	struct ratp_ctx *ctx = container_of(cdev, struct ratp_ctx, ratp_console);
> -	int len = 0;
> -
> -	len = strlen(s);
>  
>  	if (ratp_busy(&ctx->ratp))
> -		return len;
> +		return nbytes;
>  
> -	kfifo_put(ctx->console_transmit_fifo, s, len);
> +	kfifo_put(ctx->console_transmit_fifo, s, nbytes);
>  
> -	return len;
> +	return nbytes;
>  }
>  
>  static void ratp_console_putc(struct console_device *cdev, char c)
> diff --git a/drivers/serial/efi-stdio.c b/drivers/serial/efi-stdio.c
> index 0703f727e766..2ca89fa4f861 100644
> --- a/drivers/serial/efi-stdio.c
> +++ b/drivers/serial/efi-stdio.c
> @@ -243,12 +243,13 @@ static int efi_process_key(struct efi_console_priv *priv, const char *inp)
>  	return 1;
>  }
>  
> -static int efi_console_puts(struct console_device *cdev, const char *s)
> +static int efi_console_puts(struct console_device *cdev, const char *s,
> +			    size_t nbytes)
>  {
>  	struct efi_console_priv *priv = to_efi(cdev);
>  	int n = 0;
>  
> -	while (*s) {
> +	while (nbytes--) {
>  		if (*s == 27) {
>  			priv->efi_console_buffer[n] = 0;
>  			priv->out->output_string(priv->out,
> diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c
> index f0a2b22c2bc2..667d51f622ec 100644
> --- a/drivers/serial/serial_efi.c
> +++ b/drivers/serial/serial_efi.c
> @@ -130,13 +130,14 @@ static void efi_serial_putc(struct console_device *cdev, char c)
>  	serial->write(serial, &buffersize, &c);
>  }
>  
> -static int efi_serial_puts(struct console_device *cdev, const char *s)
> +static int efi_serial_puts(struct console_device *cdev, const char *s,
> +			   size_t nbytes)
>  {
>  	struct efi_serial_port *uart = to_efi_serial_port(cdev);
>  	struct efi_serial_io_protocol *serial = uart->serial;
>  	uint32_t control;
>  	efi_status_t efiret;
> -	unsigned long buffersize = strlen(s) * sizeof(char);
> +	unsigned long buffersize = nbytes;
>  
>  	do {
>  		efiret = serial->getcontrol(serial, &control);
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 755363c30999..601bfca6b025 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -67,10 +67,11 @@ static void pstore_console_write(const char *s, unsigned c)
>  	}
>  }
>  
> -static int pstore_console_puts(struct console_device *cdev, const char *s)
> +static int pstore_console_puts(struct console_device *cdev, const char *s,
> +			       size_t nbytes)
>  {
> -	pstore_console_write(s, strlen(s));
> -	return strlen(s);
> +	pstore_console_write(s, nbytes);
> +	return nbytes;
>  }
>  
>  static void pstore_console_putc(struct console_device *cdev, char c)
> diff --git a/include/console.h b/include/console.h
> index 673921331db7..1d86a584a7f2 100644
> --- a/include/console.h
> +++ b/include/console.h
> @@ -42,7 +42,7 @@ struct console_device {
>  
>  	int (*tstc)(struct console_device *cdev);
>  	void (*putc)(struct console_device *cdev, char c);
> -	int (*puts)(struct console_device *cdev, const char *s);
> +	int (*puts)(struct console_device *cdev, const char *s, size_t nbytes);
>  	int  (*getc)(struct console_device *cdev);
>  	int (*setbrg)(struct console_device *cdev, int baudrate);
>  	void (*flush)(struct console_device *cdev);
> 

Looks correct.

Bastian

-- 
Pengutronix e.K.
Industrial Linux Solutions
http://www.pengutronix.de/
Peiner Str. 6-8, 31137 Hildesheim, Germany
Amtsgericht Hildesheim, HRA 2686

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] console: fix out-of-bounds read in dputc(/dev/*, ...)
  2019-07-31  7:05 ` Bastian Krause
@ 2019-07-31  8:24   ` Ahmad Fatoum
  0 siblings, 0 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2019-07-31  8:24 UTC (permalink / raw)
  To: Bastian Krause, barebox

On 31/7/19 09:05, Bastian Krause wrote:
> On 7/30/19 5:48 PM, Ahmad Fatoum wrote:
>> Trying to output a single character via
>> 	echo -a /dev/serial0-1
>> currently results in garbage output after the newline, because console.c's
>> fops_write discards the buffer length and passes the buffer to
>> (struct cdev)::puts which only handles NUL-terminated strings.
>>
>> Fix this by amending (struct cdev)::puts with a new nbytes parameter,
>> which is correctly propagated.
>>
>> Fixes: 1233570798 ("console: expose consoles in devfs")
> 
> The is the wrong commit id, I guess you mean b4f55fcf35.

Oh, off by one (commit). Will resend.

> 
>> Cc: Bastian Krause <bst@pengutronix.de>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  common/console.c            | 12 ++++++------
>>  common/ratp/ratp.c          | 12 +++++-------
>>  drivers/serial/efi-stdio.c  |  5 +++--
>>  drivers/serial/serial_efi.c |  5 +++--
>>  fs/pstore/platform.c        |  7 ++++---
>>  include/console.h           |  2 +-
>>  6 files changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/common/console.c b/common/console.c
>> index 47ccf2e54de0..59218ea4e22f 100644
>> --- a/common/console.c
>> +++ b/common/console.c
>> @@ -253,17 +253,17 @@ static void console_set_stdoutpath(struct console_device *cdev)
>>  	free(str);
>>  }
>>  
>> -static int __console_puts(struct console_device *cdev, const char *s)
>> +static int __console_puts(struct console_device *cdev, const char *s,
>> +			  size_t nbytes)
>>  {
>> -	int n = 0;
>> +	int n;
>>  
>> -	while (*s) {
>> +	for (n = 0; n < nbytes; n++) {
>>  		if (*s == '\n') {
>>  			cdev->putc(cdev, '\r');
>>  			n++;
>>  		}
>>  		cdev->putc(cdev, *s);
>> -		n++;
>>  		s++;
>>  	}
>>  	return n;
>> @@ -298,7 +298,7 @@ static ssize_t fops_write(struct cdev* dev, const void* buf, size_t count,
>>  {
>>  	struct console_device *priv = dev->priv;
>>  
>> -	priv->puts(priv, buf);
>> +	priv->puts(priv, buf, count);
>>  
>>  	return 0;
>>  }
>> @@ -545,7 +545,7 @@ int console_puts(unsigned int ch, const char *str)
>>  	if (initialized == CONSOLE_INIT_FULL) {
>>  		for_each_console(cdev) {
>>  			if (cdev->f_active & ch) {
>> -				n = cdev->puts(cdev, str);
>> +				n = cdev->puts(cdev, str, strlen(str));
>>  			}
>>  		}
>>  		return n;
>> diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
>> index 9aea1786d684..e84ad221672a 100644
>> --- a/common/ratp/ratp.c
>> +++ b/common/ratp/ratp.c
>> @@ -259,19 +259,17 @@ static int ratp_console_tstc(struct console_device *cdev)
>>  	return kfifo_len(ctx->console_recv_fifo) ? 1 : 0;
>>  }
>>  
>> -static int ratp_console_puts(struct console_device *cdev, const char *s)
>> +static int ratp_console_puts(struct console_device *cdev, const char *s,
>> +			     size_t nbytes)
>>  {
>>  	struct ratp_ctx *ctx = container_of(cdev, struct ratp_ctx, ratp_console);
>> -	int len = 0;
>> -
>> -	len = strlen(s);
>>  
>>  	if (ratp_busy(&ctx->ratp))
>> -		return len;
>> +		return nbytes;
>>  
>> -	kfifo_put(ctx->console_transmit_fifo, s, len);
>> +	kfifo_put(ctx->console_transmit_fifo, s, nbytes);
>>  
>> -	return len;
>> +	return nbytes;
>>  }
>>  
>>  static void ratp_console_putc(struct console_device *cdev, char c)
>> diff --git a/drivers/serial/efi-stdio.c b/drivers/serial/efi-stdio.c
>> index 0703f727e766..2ca89fa4f861 100644
>> --- a/drivers/serial/efi-stdio.c
>> +++ b/drivers/serial/efi-stdio.c
>> @@ -243,12 +243,13 @@ static int efi_process_key(struct efi_console_priv *priv, const char *inp)
>>  	return 1;
>>  }
>>  
>> -static int efi_console_puts(struct console_device *cdev, const char *s)
>> +static int efi_console_puts(struct console_device *cdev, const char *s,
>> +			    size_t nbytes)
>>  {
>>  	struct efi_console_priv *priv = to_efi(cdev);
>>  	int n = 0;
>>  
>> -	while (*s) {
>> +	while (nbytes--) {
>>  		if (*s == 27) {
>>  			priv->efi_console_buffer[n] = 0;
>>  			priv->out->output_string(priv->out,
>> diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c
>> index f0a2b22c2bc2..667d51f622ec 100644
>> --- a/drivers/serial/serial_efi.c
>> +++ b/drivers/serial/serial_efi.c
>> @@ -130,13 +130,14 @@ static void efi_serial_putc(struct console_device *cdev, char c)
>>  	serial->write(serial, &buffersize, &c);
>>  }
>>  
>> -static int efi_serial_puts(struct console_device *cdev, const char *s)
>> +static int efi_serial_puts(struct console_device *cdev, const char *s,
>> +			   size_t nbytes)
>>  {
>>  	struct efi_serial_port *uart = to_efi_serial_port(cdev);
>>  	struct efi_serial_io_protocol *serial = uart->serial;
>>  	uint32_t control;
>>  	efi_status_t efiret;
>> -	unsigned long buffersize = strlen(s) * sizeof(char);
>> +	unsigned long buffersize = nbytes;
>>  
>>  	do {
>>  		efiret = serial->getcontrol(serial, &control);
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index 755363c30999..601bfca6b025 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -67,10 +67,11 @@ static void pstore_console_write(const char *s, unsigned c)
>>  	}
>>  }
>>  
>> -static int pstore_console_puts(struct console_device *cdev, const char *s)
>> +static int pstore_console_puts(struct console_device *cdev, const char *s,
>> +			       size_t nbytes)
>>  {
>> -	pstore_console_write(s, strlen(s));
>> -	return strlen(s);
>> +	pstore_console_write(s, nbytes);
>> +	return nbytes;
>>  }
>>  
>>  static void pstore_console_putc(struct console_device *cdev, char c)
>> diff --git a/include/console.h b/include/console.h
>> index 673921331db7..1d86a584a7f2 100644
>> --- a/include/console.h
>> +++ b/include/console.h
>> @@ -42,7 +42,7 @@ struct console_device {
>>  
>>  	int (*tstc)(struct console_device *cdev);
>>  	void (*putc)(struct console_device *cdev, char c);
>> -	int (*puts)(struct console_device *cdev, const char *s);
>> +	int (*puts)(struct console_device *cdev, const char *s, size_t nbytes);
>>  	int  (*getc)(struct console_device *cdev);
>>  	int (*setbrg)(struct console_device *cdev, int baudrate);
>>  	void (*flush)(struct console_device *cdev);
>>
> 
> Looks correct.
> 
> Bastian
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] console: fix out-of-bounds read in dputc(/dev/*, ...)
  2019-07-30 15:53 ` Ahmad Fatoum
@ 2019-07-31 10:23   ` Ahmad Fatoum
  0 siblings, 0 replies; 5+ messages in thread
From: Ahmad Fatoum @ 2019-07-31 10:23 UTC (permalink / raw)
  To: barebox

On 30/7/19 17:53, Ahmad Fatoum wrote:
> On 30/7/19 17:48, Ahmad Fatoum wrote:
>> Trying to output a single character via
>> 	echo -a /dev/serial0-1
>> currently results in garbage output after the newline, because console.c's
>> fops_write discards the buffer length and passes the buffer to
>> (struct cdev)::puts which only handles NUL-terminated strings.
>>
>> Fix this by amending (struct cdev)::puts with a new nbytes parameter,
>> which is correctly propagated.
>>
>> Fixes: 1233570798 ("console: expose consoles in devfs")
>> Cc: Bastian Krause <bst@pengutronix.de>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  common/console.c            | 12 ++++++------
>>  common/ratp/ratp.c          | 12 +++++-------
>>  drivers/serial/efi-stdio.c  |  5 +++--
>>  drivers/serial/serial_efi.c |  5 +++--
>>  fs/pstore/platform.c        |  7 ++++---
>>  include/console.h           |  2 +-
>>  6 files changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/common/console.c b/common/console.c
>> index 47ccf2e54de0..59218ea4e22f 100644
>> --- a/common/console.c
>> +++ b/common/console.c
>> @@ -253,17 +253,17 @@ static void console_set_stdoutpath(struct console_device *cdev)
>>  	free(str);
>>  }
>>  
>> -static int __console_puts(struct console_device *cdev, const char *s)
>> +static int __console_puts(struct console_device *cdev, const char *s,
>> +			  size_t nbytes)
>>  {
>> -	int n = 0;
>> +	int n;
>>  
>> -	while (*s) {
>> +	for (n = 0; n < nbytes; n++) {
>>  		if (*s == '\n') {
>>  			cdev->putc(cdev, '\r');
>>  			n++;
>>  		}
>>  		cdev->putc(cdev, *s);
>> -		n++;
>>  		s++;
>>  	}
>>  	return n;
>> @@ -298,7 +298,7 @@ static ssize_t fops_write(struct cdev* dev, const void* buf, size_t count,
>>  {
>>  	struct console_device *priv = dev->priv;
>>  
>> -	priv->puts(priv, buf);
>> +	priv->puts(priv, buf, count);
>>  
>>  	return 0;
>>  }
>> @@ -545,7 +545,7 @@ int console_puts(unsigned int ch, const char *str)
>>  	if (initialized == CONSOLE_INIT_FULL) {
>>  		for_each_console(cdev) {
>>  			if (cdev->f_active & ch) {
>> -				n = cdev->puts(cdev, str);
>> +				n = cdev->puts(cdev, str, strlen(str));
>>  			}
>>  		}
>>  		return n;
>> diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
>> index 9aea1786d684..e84ad221672a 100644
>> --- a/common/ratp/ratp.c
>> +++ b/common/ratp/ratp.c
>> @@ -259,19 +259,17 @@ static int ratp_console_tstc(struct console_device *cdev)
>>  	return kfifo_len(ctx->console_recv_fifo) ? 1 : 0;
>>  }
>>  
>> -static int ratp_console_puts(struct console_device *cdev, const char *s)
>> +static int ratp_console_puts(struct console_device *cdev, const char *s,
>> +			     size_t nbytes)
>>  {
>>  	struct ratp_ctx *ctx = container_of(cdev, struct ratp_ctx, ratp_console);
>> -	int len = 0;
>> -
>> -	len = strlen(s);
>>  
>>  	if (ratp_busy(&ctx->ratp))
>> -		return len;
>> +		return nbytes;
> 
> This didn't look right before. Should this be a return 0 instead?
> Sascha?

Changed to zero in v2.

> 
>>  
>> -	kfifo_put(ctx->console_transmit_fifo, s, len);
>> +	kfifo_put(ctx->console_transmit_fifo, s, nbytes);
>>  
>> -	return len;
>> +	return nbytes;
>>  }
>>  
>>  static void ratp_console_putc(struct console_device *cdev, char c)
>> diff --git a/drivers/serial/efi-stdio.c b/drivers/serial/efi-stdio.c
>> index 0703f727e766..2ca89fa4f861 100644
>> --- a/drivers/serial/efi-stdio.c
>> +++ b/drivers/serial/efi-stdio.c
>> @@ -243,12 +243,13 @@ static int efi_process_key(struct efi_console_priv *priv, const char *inp)
>>  	return 1;
>>  }
>>  
>> -static int efi_console_puts(struct console_device *cdev, const char *s)
>> +static int efi_console_puts(struct console_device *cdev, const char *s,
>> +			    size_t nbytes)
>>  {
>>  	struct efi_console_priv *priv = to_efi(cdev);
>>  	int n = 0;
>>  
>> -	while (*s) {
>> +	while (nbytes--) {
>>  		if (*s == 27) {
>>  			priv->efi_console_buffer[n] = 0;
>>  			priv->out->output_string(priv->out,
>> diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c
>> index f0a2b22c2bc2..667d51f622ec 100644
>> --- a/drivers/serial/serial_efi.c
>> +++ b/drivers/serial/serial_efi.c
>> @@ -130,13 +130,14 @@ static void efi_serial_putc(struct console_device *cdev, char c)
>>  	serial->write(serial, &buffersize, &c);
>>  }
>>  
>> -static int efi_serial_puts(struct console_device *cdev, const char *s)
>> +static int efi_serial_puts(struct console_device *cdev, const char *s,
>> +			   size_t nbytes)
>>  {
>>  	struct efi_serial_port *uart = to_efi_serial_port(cdev);
>>  	struct efi_serial_io_protocol *serial = uart->serial;
>>  	uint32_t control;
>>  	efi_status_t efiret;
>> -	unsigned long buffersize = strlen(s) * sizeof(char);
>> +	unsigned long buffersize = nbytes;
>>  
>>  	do {
>>  		efiret = serial->getcontrol(serial, &control);
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index 755363c30999..601bfca6b025 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -67,10 +67,11 @@ static void pstore_console_write(const char *s, unsigned c)
>>  	}
>>  }
>>  
>> -static int pstore_console_puts(struct console_device *cdev, const char *s)
>> +static int pstore_console_puts(struct console_device *cdev, const char *s,
>> +			       size_t nbytes)
>>  {
>> -	pstore_console_write(s, strlen(s));
>> -	return strlen(s);
>> +	pstore_console_write(s, nbytes);
>> +	return nbytes;
>>  }
>>  
>>  static void pstore_console_putc(struct console_device *cdev, char c)
>> diff --git a/include/console.h b/include/console.h
>> index 673921331db7..1d86a584a7f2 100644
>> --- a/include/console.h
>> +++ b/include/console.h
>> @@ -42,7 +42,7 @@ struct console_device {
>>  
>>  	int (*tstc)(struct console_device *cdev);
>>  	void (*putc)(struct console_device *cdev, char c);
>> -	int (*puts)(struct console_device *cdev, const char *s);
>> +	int (*puts)(struct console_device *cdev, const char *s, size_t nbytes);
> 
> Thoughts on renaming this to something else? Apparently non-matching function
> pointers are just a warning with gcc 8.2.1 (OSELAS.Toolchain-2018.12.0).
> I'd have preferred non-mainline serial drivers to completely fail to compile.

Same concern for v2

> 
>>  	int  (*getc)(struct console_device *cdev);
>>  	int (*setbrg)(struct console_device *cdev, int baudrate);
>>  	void (*flush)(struct console_device *cdev);
>>
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-31 10:23 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30 15:48 [PATCH] console: fix out-of-bounds read in dputc(/dev/*, ...) Ahmad Fatoum
2019-07-30 15:53 ` Ahmad Fatoum
2019-07-31 10:23   ` Ahmad Fatoum
2019-07-31  7:05 ` Bastian Krause
2019-07-31  8:24   ` Ahmad Fatoum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox