[PATCH 0/2] serial ma35d1 imrpovements

[PATCH 0/2] serial ma35d1 imrpovements

[Andi Shyti]

Hi,

Just a couple of improvements after running through a failure
for the serial ma35d1 driver.

Thanks,
Andi

Andi Shyti (2):
  serial: ma35d1: Validate console index before assignment
  serial: ma35d1: Improve logging for out-of-bound console setup

 drivers/tty/serial/ma35d1_serial.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/2] serial: ma35d1: Validate console index before assignment

[Andi Shyti]

The console is immediately assigned to the ma35d1 port without
checking its index. This oversight can lead to out-of-bounds
errors when the index falls outside the valid '0' to
MA35_UART_NR range. Such scenario trigges ran error like the
following:

 UBSAN: array-index-out-of-bounds in drivers/tty/serial/ma35d1_serial.c:555:51
 index -1 is out of range for type 'uart_ma35d1_port [17]

Check the index before using it and bail out with a warning.

Fixes: 930cbf92db01 ("tty: serial: Add Nuvoton ma35d1 serial driver support")
Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
Cc: Jacky Huang <ychuang3@nuvoton.com>
Cc: <stable@vger.kernel.org> # v6.5+
---
 drivers/tty/serial/ma35d1_serial.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/ma35d1_serial.c b/drivers/tty/serial/ma35d1_serial.c
index a6a7c405892e8..21b574f78b861 100644
--- a/drivers/tty/serial/ma35d1_serial.c
+++ b/drivers/tty/serial/ma35d1_serial.c
@@ -552,11 +552,19 @@ static void ma35d1serial_console_putchar(struct uart_port *port, unsigned char c
  */
 static void ma35d1serial_console_write(struct console *co, const char *s, u32 count)
 {
-	struct uart_ma35d1_port *up = &ma35d1serial_ports[co->index];
+	struct uart_ma35d1_port *up;
 	unsigned long flags;
 	int locked = 1;
 	u32 ier;
 
+	if ((co->index < 0) || (co->index >= MA35_UART_NR)) {
+		pr_warn("Failed to write on ononsole port %x, out of range\n",
+			co->index);
+		return;
+	}
+
+	up = &ma35d1serial_ports[co->index];
+
 	if (up->port.sysrq)
 		locked = 0;
 	else if (oops_in_progress)
-- 
2.43.0

[PATCH 2/2] serial: ma35d1: Improve logging for out-of-bound console setup

[Andi Shyti]

An out-of-bound index results in an error and should not be
logged merely as a debug message; it requires at least a warning
level. Therefore, use pr_warn() instead of pr_debug.

Additionally, the log message itself has been improved for
clarity.

Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
Cc: Jacky Huang <ychuang3@nuvoton.com>
---
 drivers/tty/serial/ma35d1_serial.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/ma35d1_serial.c b/drivers/tty/serial/ma35d1_serial.c
index 21b574f78b861..bcc402b4c0b2f 100644
--- a/drivers/tty/serial/ma35d1_serial.c
+++ b/drivers/tty/serial/ma35d1_serial.c
@@ -599,7 +599,8 @@ static int __init ma35d1serial_console_setup(struct console *co, char *options)
 	int flow = 'n';
 
 	if ((co->index < 0) || (co->index >= MA35_UART_NR)) {
-		pr_debug("Console Port%x out of range\n", co->index);
+		pr_warn("Failed to write on cononsole port %x, out of range\n",
+			co->index);
 		return -EINVAL;
 	}
 
-- 
2.43.0

Re: [PATCH 2/2] serial: ma35d1: Improve logging for out-of-bound console setup

[Hugo Villeneuve]

On Mon,  4 Dec 2023 17:38:04 +0100
Andi Shyti <andi.shyti@kernel.org> wrote:

> An out-of-bound index results in an error and should not be
> logged merely as a debug message; it requires at least a warning
> level. Therefore, use pr_warn() instead of pr_debug.
> 
> Additionally, the log message itself has been improved for
> clarity.
> 
> Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
> Cc: Jacky Huang <ychuang3@nuvoton.com>
> ---
>  drivers/tty/serial/ma35d1_serial.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/ma35d1_serial.c b/drivers/tty/serial/ma35d1_serial.c
> index 21b574f78b861..bcc402b4c0b2f 100644
> --- a/drivers/tty/serial/ma35d1_serial.c
> +++ b/drivers/tty/serial/ma35d1_serial.c
> @@ -599,7 +599,8 @@ static int __init ma35d1serial_console_setup(struct console *co, char *options)
>  	int flow = 'n';
>  
>  	if ((co->index < 0) || (co->index >= MA35_UART_NR)) {
> -		pr_debug("Console Port%x out of range\n", co->index);
> +		pr_warn("Failed to write on cononsole port %x, out of range\n",
> +			co->index);

Hi,
I do not see why this improves clarity...

You also introduced a syntax error "cononsole".

Hugo.


>  		return -EINVAL;
>  	}
>  
> -- 
> 2.43.0
> 
> 


-- 
Hugo Villeneuve

Re: [PATCH 2/2] serial: ma35d1: Improve logging for out-of-bound console setup

[Andi Shyti]

Hi Hugo,

> Andi Shyti <andi.shyti@kernel.org> wrote:
> > An out-of-bound index results in an error and should not be
> > logged merely as a debug message; it requires at least a warning
> > level. Therefore, use pr_warn() instead of pr_debug.
> > 
> > Additionally, the log message itself has been improved for
> > clarity.
> > 
> > Signed-off-by: Andi Shyti <andi.shyti@kernel.org>
> > Cc: Jacky Huang <ychuang3@nuvoton.com>
> > ---
> >  drivers/tty/serial/ma35d1_serial.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/ma35d1_serial.c b/drivers/tty/serial/ma35d1_serial.c
> > index 21b574f78b861..bcc402b4c0b2f 100644
> > --- a/drivers/tty/serial/ma35d1_serial.c
> > +++ b/drivers/tty/serial/ma35d1_serial.c
> > @@ -599,7 +599,8 @@ static int __init ma35d1serial_console_setup(struct console *co, char *options)
> >  	int flow = 'n';
> >  
> >  	if ((co->index < 0) || (co->index >= MA35_UART_NR)) {
> > -		pr_debug("Console Port%x out of range\n", co->index);
> > +		pr_warn("Failed to write on cononsole port %x, out of range\n",
> > +			co->index);
> 
> Hi,
> I do not see why this improves clarity...

I came here from the previous patch (which, by the way, needs a
little change) as I noticed that the same check was being
performed elsewhere, but with a different message. I decided I
prefer my version more because if the id is 18, 'Console Port18
out of range' doesn’t mean much to me, and I don't know what the
driver is trying to do.

Another improvement to the message would be to print out the
failing function.

Of course, I can live without changing the message.

> 
> You also introduced a syntax error "cononsole".

that's unwanted, thanks.

Andi

> Hugo.
> 
> 
> >  		return -EINVAL;
> >  	}
> >  
> > -- 
> > 2.43.0
> > 
> > 
> 
> 
> -- 
> Hugo Villeneuve