All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-04-26 15:06 ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-04-26 15:06 UTC (permalink / raw)
  To: linux-serial
  Cc: Alexandre Belloni, Peter Hurley, Richard Genoud,
	Greg Kroah-Hartman, Jiri Slaby, tglx, linux-arm-kernel

I was puzzled while looking at /proc/interrupts and random things showed
up between reboots. This occurred more often but I realised it later. The
"correct" output should be:
|38:      11861  atmel-aic5   2 Level     ttyS0

but I saw sometimes
|38:       6426  atmel-aic5   2 Level     tty1

and I accounted it wrongly as correct. This is use after free and the
former example randomly got the "old" pointer which pointed to the same
content. With SLAB_FREELIST_RANDOM and HARDENED I even got
|38:       7067  atmel-aic5   2 Level     E=Started User Manager for UID 0

or other nonsense.
As it turns out the tty, pointer that is accessed in atmel_startup(), is
freed() before atmel_shutdown(). It seems to happen quite often that the
tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
don't do anything special - just a systemd boot :)

It seems not to happen in v4.1.51 but it happens in v4.9 and v4.17-rc2
so if it broke accidentally it was not recently.

Use port->name as the IRQ name for request_irq(). This exists as long as
the driver is loaded so no use-after-free here.

Cc: stable@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/atmel_serial.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e287fe8f10fc..d3189816740e 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1757,7 +1757,6 @@ static int atmel_startup(struct uart_port *port)
 {
 	struct platform_device *pdev = to_platform_device(port->dev);
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
-	struct tty_struct *tty = port->state->port.tty;
 	int retval;
 
 	/*
@@ -1772,8 +1771,7 @@ static int atmel_startup(struct uart_port *port)
 	 * Allocate the IRQ
 	 */
 	retval = request_irq(port->irq, atmel_interrupt,
-			IRQF_SHARED | IRQF_COND_SUSPEND,
-			tty ? tty->name : "atmel_serial", port);
+			     IRQF_SHARED | IRQF_COND_SUSPEND, port->name, port);
 	if (retval) {
 		dev_err(port->dev, "atmel_startup - Can't get irq\n");
 		return retval;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-04-26 15:06 ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-04-26 15:06 UTC (permalink / raw)
  To: linux-arm-kernel

I was puzzled while looking at /proc/interrupts and random things showed
up between reboots. This occurred more often but I realised it later. The
"correct" output should be:
|38:      11861  atmel-aic5   2 Level     ttyS0

but I saw sometimes
|38:       6426  atmel-aic5   2 Level     tty1

and I accounted it wrongly as correct. This is use after free and the
former example randomly got the "old" pointer which pointed to the same
content. With SLAB_FREELIST_RANDOM and HARDENED I even got
|38:       7067  atmel-aic5   2 Level     E=Started User Manager for UID 0

or other nonsense.
As it turns out the tty, pointer that is accessed in atmel_startup(), is
freed() before atmel_shutdown(). It seems to happen quite often that the
tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
don't do anything special - just a systemd boot :)

It seems not to happen in v4.1.51 but it happens in v4.9 and v4.17-rc2
so if it broke accidentally it was not recently.

Use port->name as the IRQ name for request_irq(). This exists as long as
the driver is loaded so no use-after-free here.

Cc: stable at vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/atmel_serial.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e287fe8f10fc..d3189816740e 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1757,7 +1757,6 @@ static int atmel_startup(struct uart_port *port)
 {
 	struct platform_device *pdev = to_platform_device(port->dev);
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
-	struct tty_struct *tty = port->state->port.tty;
 	int retval;
 
 	/*
@@ -1772,8 +1771,7 @@ static int atmel_startup(struct uart_port *port)
 	 * Allocate the IRQ
 	 */
 	retval = request_irq(port->irq, atmel_interrupt,
-			IRQF_SHARED | IRQF_COND_SUSPEND,
-			tty ? tty->name : "atmel_serial", port);
+			     IRQF_SHARED | IRQF_COND_SUSPEND, port->name, port);
 	if (retval) {
 		dev_err(port->dev, "atmel_startup - Can't get irq\n");
 		return retval;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] tty/serial: atmel: use port->name as name in request_irq()
  2018-04-26 15:06 ` Sebastian Andrzej Siewior
@ 2018-04-26 15:12   ` Sebastian Andrzej Siewior
  -1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-04-26 15:12 UTC (permalink / raw)
  To: linux-serial
  Cc: Alexandre Belloni, Peter Hurley, Richard Genoud,
	Greg Kroah-Hartman, Jiri Slaby, tglx, linux-arm-kernel

On 2018-04-26 17:06:25 [+0200], To linux-serial@vger.kernel.org wrote:
> It seems not to happen in v4.1.51 but it happens in v4.9 and v4.17-rc2
> so if it broke accidentally it was not recently.

This is what I used to check:

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -170,7 +170,7 @@ void free_tty_struct(struct tty_struct *tty)
 	put_device(tty->dev);
 	kfree(tty->write_buf);
 	tty->magic = 0xDEADDEAD;
-	kfree(tty);
+	strcpy(tty->name, "GONE");
 }
 
 static inline struct tty_struct *file_tty(struct file *file)

If this is unknown and a bisect is requested, please let me know.

Sebastian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-04-26 15:12   ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-04-26 15:12 UTC (permalink / raw)
  To: linux-arm-kernel

On 2018-04-26 17:06:25 [+0200], To linux-serial at vger.kernel.org wrote:
> It seems not to happen in v4.1.51 but it happens in v4.9 and v4.17-rc2
> so if it broke accidentally it was not recently.

This is what I used to check:

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -170,7 +170,7 @@ void free_tty_struct(struct tty_struct *tty)
 	put_device(tty->dev);
 	kfree(tty->write_buf);
 	tty->magic = 0xDEADDEAD;
-	kfree(tty);
+	strcpy(tty->name, "GONE");
 }
 
 static inline struct tty_struct *file_tty(struct file *file)

If this is unknown and a bisect is requested, please let me know.

Sebastian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] tty/serial: atmel: use port->name as name in request_irq()
  2018-04-26 15:12   ` Sebastian Andrzej Siewior
@ 2018-04-27 10:31     ` Richard Genoud
  -1 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-04-27 10:31 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, linux-serial
  Cc: Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman, Jiri Slaby,
	tglx, linux-arm-kernel

Hi Sebastian,

On 26/04/2018 17:12, Sebastian Andrzej Siewior wrote:
> On 2018-04-26 17:06:25 [+0200], To linux-serial@vger.kernel.org wrote:
>> It seems not to happen in v4.1.51 but it happens in v4.9 and v4.17-rc2
>> so if it broke accidentally it was not recently.
> 
> This is what I used to check:
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -170,7 +170,7 @@ void free_tty_struct(struct tty_struct *tty)
>  	put_device(tty->dev);
>  	kfree(tty->write_buf);
>  	tty->magic = 0xDEADDEAD;
> -	kfree(tty);
> +	strcpy(tty->name, "GONE");
>  }
>  
>  static inline struct tty_struct *file_tty(struct file *file)
> 
> If this is unknown and a bisect is requested, please let me know.
Indeed, this will be appreciated.
I'm quite curious to find the commit that led to this.

Thanks !

> 
> Sebastian
> 

Richard

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-04-27 10:31     ` Richard Genoud
  0 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-04-27 10:31 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Sebastian,

On 26/04/2018 17:12, Sebastian Andrzej Siewior wrote:
> On 2018-04-26 17:06:25 [+0200], To linux-serial at vger.kernel.org wrote:
>> It seems not to happen in v4.1.51 but it happens in v4.9 and v4.17-rc2
>> so if it broke accidentally it was not recently.
> 
> This is what I used to check:
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -170,7 +170,7 @@ void free_tty_struct(struct tty_struct *tty)
>  	put_device(tty->dev);
>  	kfree(tty->write_buf);
>  	tty->magic = 0xDEADDEAD;
> -	kfree(tty);
> +	strcpy(tty->name, "GONE");
>  }
>  
>  static inline struct tty_struct *file_tty(struct file *file)
> 
> If this is unknown and a bisect is requested, please let me know.
Indeed, this will be appreciated.
I'm quite curious to find the commit that led to this.

Thanks !

> 
> Sebastian
> 

Richard

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] tty/serial: atmel: use port->name as name in request_irq()
  2018-04-27 10:31     ` Richard Genoud
@ 2018-05-02 19:16       ` Sebastian Andrzej Siewior
  -1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-02 19:16 UTC (permalink / raw)
  To: Richard Genoud
  Cc: Rob Herring, Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman,
	linux-serial, Jiri Slaby, tglx, linux-arm-kernel

On 2018-04-27 12:31:52 [+0200], Richard Genoud wrote:
> Hi Sebastian,
Hi,

> > If this is unknown and a bisect is requested, please let me know.
> Indeed, this will be appreciated.
> I'm quite curious to find the commit that led to this.

commit 761ed4a94582ab291aa24dcbea4e01e8936488c8
Author: Rob Herring <robh@kernel.org>
Date:   Mon Aug 22 17:39:10 2016 -0500

    tty: serial_core: convert uart_close to use tty_port_close
    
    tty_port_close handles much of the common parts of tty close. Convert
    uart_close to use it and move the serial_core specific parts into
    tty_port.shutdown function. This will be needed to use tty_port functions
    directly from in kernel clients.
    
    This change causes ops->stop_rx() to be called after uart_wait_until_sent()
    is called which I think should be fine. Otherwise, the sequence of the
    close should be the same.
    
    Cc: Peter Hurley <peter@hurleysoftware.com>
    Signed-off-by: Rob Herring <robh@kernel.org>
    Acked-by: Alan Cox <alan@linux.intel.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

The thread starts at
  https://www.spinics.net/lists/linux-serial/msg30070.html
  http://lkml.kernel.org/r/20180426150625.q5tqcb7fzchvkb5d@linutronix.de

> Richard

Sebastian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-05-02 19:16       ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-02 19:16 UTC (permalink / raw)
  To: linux-arm-kernel

On 2018-04-27 12:31:52 [+0200], Richard Genoud wrote:
> Hi Sebastian,
Hi,

> > If this is unknown and a bisect is requested, please let me know.
> Indeed, this will be appreciated.
> I'm quite curious to find the commit that led to this.

commit 761ed4a94582ab291aa24dcbea4e01e8936488c8
Author: Rob Herring <robh@kernel.org>
Date:   Mon Aug 22 17:39:10 2016 -0500

    tty: serial_core: convert uart_close to use tty_port_close
    
    tty_port_close handles much of the common parts of tty close. Convert
    uart_close to use it and move the serial_core specific parts into
    tty_port.shutdown function. This will be needed to use tty_port functions
    directly from in kernel clients.
    
    This change causes ops->stop_rx() to be called after uart_wait_until_sent()
    is called which I think should be fine. Otherwise, the sequence of the
    close should be the same.
    
    Cc: Peter Hurley <peter@hurleysoftware.com>
    Signed-off-by: Rob Herring <robh@kernel.org>
    Acked-by: Alan Cox <alan@linux.intel.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

The thread starts at
  https://www.spinics.net/lists/linux-serial/msg30070.html
  http://lkml.kernel.org/r/20180426150625.q5tqcb7fzchvkb5d at linutronix.de

> Richard

Sebastian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] tty/serial: atmel: use port->name as name in request_irq()
  2018-05-02 19:16       ` Sebastian Andrzej Siewior
@ 2018-05-03 12:36         ` Richard Genoud
  -1 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-05-03 12:36 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Rob Herring, Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman,
	linux-serial, Jiri Slaby, tglx, linux-arm-kernel

On 02/05/2018 21:16, Sebastian Andrzej Siewior wrote:
> On 2018-04-27 12:31:52 [+0200], Richard Genoud wrote:
>> Hi Sebastian,
> Hi,
> 
>>> If this is unknown and a bisect is requested, please let me know.
>> Indeed, this will be appreciated.
>> I'm quite curious to find the commit that led to this.
> 
> commit 761ed4a94582ab291aa24dcbea4e01e8936488c8
> Author: Rob Herring <robh@kernel.org>
> Date:   Mon Aug 22 17:39:10 2016 -0500
> 
>     tty: serial_core: convert uart_close to use tty_port_close
>     
>     tty_port_close handles much of the common parts of tty close. Convert
>     uart_close to use it and move the serial_core specific parts into
>     tty_port.shutdown function. This will be needed to use tty_port functions
>     directly from in kernel clients.
>     
>     This change causes ops->stop_rx() to be called after uart_wait_until_sent()
>     is called which I think should be fine. Otherwise, the sequence of the
>     close should be the same.
>     
>     Cc: Peter Hurley <peter@hurleysoftware.com>
>     Signed-off-by: Rob Herring <robh@kernel.org>
>     Acked-by: Alan Cox <alan@linux.intel.com>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> The thread starts at
>   https://www.spinics.net/lists/linux-serial/msg30070.html
>   http://lkml.kernel.org/r/20180426150625.q5tqcb7fzchvkb5d@linutronix.de

Thanks for this hunting session !
Could resend your patch with Fixes: in the commit message ?

BTW, I didn't manage to reproduce the behavior you describe, could you
give me your .config and describe a little more how you manage to
trigger this bug ?
(do you use the console on serial debug ? which board ? )


Thanks !

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-05-03 12:36         ` Richard Genoud
  0 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-05-03 12:36 UTC (permalink / raw)
  To: linux-arm-kernel

On 02/05/2018 21:16, Sebastian Andrzej Siewior wrote:
> On 2018-04-27 12:31:52 [+0200], Richard Genoud wrote:
>> Hi Sebastian,
> Hi,
> 
>>> If this is unknown and a bisect is requested, please let me know.
>> Indeed, this will be appreciated.
>> I'm quite curious to find the commit that led to this.
> 
> commit 761ed4a94582ab291aa24dcbea4e01e8936488c8
> Author: Rob Herring <robh@kernel.org>
> Date:   Mon Aug 22 17:39:10 2016 -0500
> 
>     tty: serial_core: convert uart_close to use tty_port_close
>     
>     tty_port_close handles much of the common parts of tty close. Convert
>     uart_close to use it and move the serial_core specific parts into
>     tty_port.shutdown function. This will be needed to use tty_port functions
>     directly from in kernel clients.
>     
>     This change causes ops->stop_rx() to be called after uart_wait_until_sent()
>     is called which I think should be fine. Otherwise, the sequence of the
>     close should be the same.
>     
>     Cc: Peter Hurley <peter@hurleysoftware.com>
>     Signed-off-by: Rob Herring <robh@kernel.org>
>     Acked-by: Alan Cox <alan@linux.intel.com>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> The thread starts at
>   https://www.spinics.net/lists/linux-serial/msg30070.html
>   http://lkml.kernel.org/r/20180426150625.q5tqcb7fzchvkb5d at linutronix.de

Thanks for this hunting session !
Could resend your patch with Fixes: in the commit message ?

BTW, I didn't manage to reproduce the behavior you describe, could you
give me your .config and describe a little more how you manage to
trigger this bug ?
(do you use the console on serial debug ? which board ? )


Thanks !

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] tty/serial: atmel: use port->name as name in request_irq()
  2018-05-03 12:36         ` Richard Genoud
@ 2018-05-03 12:44           ` Sebastian Andrzej Siewior
  -1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-03 12:44 UTC (permalink / raw)
  To: Richard Genoud
  Cc: Rob Herring, Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman,
	linux-serial, Jiri Slaby, tglx, linux-arm-kernel

On 2018-05-03 14:36:03 [+0200], Richard Genoud wrote:
> Could resend your patch with Fixes: in the commit message ?

will do shortly.

> BTW, I didn't manage to reproduce the behavior you describe, could you
> give me your .config and describe a little more how you manage to
> trigger this bug ?
.config sent offlist.
Did you not reproduce this even with the second/debug patch I've sent?

> (do you use the console on serial debug ? which board ? )
up to date debian sid booting with systemd. After boot completed I do
"cat /proc/interrupts" and check for the "gone" string.
This is
[    0.000000] OF: fdt:Machine model: SAMA5D3 Xplained
[    0.000000] Kernel command line: earlyprintk=ttyS0,115200 console=ttyS0,115200 root=/dev/mmcblk0p2 rootwait
[    0.520000] AT91: Detected SoC family: sama5d3
[    0.520000] AT91: Detected SoC: sama5d36, revision 2

at91-sama5d3_xplained.dtb

Is this enough?

> Thanks !

Sebastian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-05-03 12:44           ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-03 12:44 UTC (permalink / raw)
  To: linux-arm-kernel

On 2018-05-03 14:36:03 [+0200], Richard Genoud wrote:
> Could resend your patch with Fixes: in the commit message ?

will do shortly.

> BTW, I didn't manage to reproduce the behavior you describe, could you
> give me your .config and describe a little more how you manage to
> trigger this bug ?
.config sent offlist.
Did you not reproduce this even with the second/debug patch I've sent?

> (do you use the console on serial debug ? which board ? )
up to date debian sid booting with systemd. After boot completed I do
"cat /proc/interrupts" and check for the "gone" string.
This is
[    0.000000] OF: fdt:Machine model: SAMA5D3 Xplained
[    0.000000] Kernel command line: earlyprintk=ttyS0,115200 console=ttyS0,115200 root=/dev/mmcblk0p2 rootwait
[    0.520000] AT91: Detected SoC family: sama5d3
[    0.520000] AT91: Detected SoC: sama5d36, revision 2

at91-sama5d3_xplained.dtb

Is this enough?

> Thanks !

Sebastian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] tty/serial: atmel: use port->name as name in request_irq()
  2018-05-03 12:44           ` Sebastian Andrzej Siewior
@ 2018-05-03 13:34             ` Richard Genoud
  -1 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-05-03 13:34 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, Richard Genoud
  Cc: Rob Herring, Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman,
	linux-serial, Jiri Slaby, tglx, linux-arm-kernel

On 03/05/2018 14:44, Sebastian Andrzej Siewior wrote:
> On 2018-05-03 14:36:03 [+0200], Richard Genoud wrote:
>> Could resend your patch with Fixes: in the commit message ?
> 
> will do shortly.
> 
>> BTW, I didn't manage to reproduce the behavior you describe, could you
>> give me your .config and describe a little more how you manage to
>> trigger this bug ?
> .config sent offlist.
> Did you not reproduce this even with the second/debug patch I've sent?
Nope. (I tried on a sam9g35 with buildroot/busybox)

I have a sama5d3 with a debian as a home server, I'll give it a try.

> 
>> (do you use the console on serial debug ? which board ? )
> up to date debian sid booting with systemd. After boot completed I do
> "cat /proc/interrupts" and check for the "gone" string.
> This is
> [    0.000000] OF: fdt:Machine model: SAMA5D3 Xplained
> [    0.000000] Kernel command line: earlyprintk=ttyS0,115200 console=ttyS0,115200 root=/dev/mmcblk0p2 rootwait
> [    0.520000] AT91: Detected SoC family: sama5d3
> [    0.520000] AT91: Detected SoC: sama5d36, revision 2
> 
> at91-sama5d3_xplained.dtb
> 
> Is this enough?

Great,

Thanks !

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-05-03 13:34             ` Richard Genoud
  0 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-05-03 13:34 UTC (permalink / raw)
  To: linux-arm-kernel

On 03/05/2018 14:44, Sebastian Andrzej Siewior wrote:
> On 2018-05-03 14:36:03 [+0200], Richard Genoud wrote:
>> Could resend your patch with Fixes: in the commit message ?
> 
> will do shortly.
> 
>> BTW, I didn't manage to reproduce the behavior you describe, could you
>> give me your .config and describe a little more how you manage to
>> trigger this bug ?
> .config sent offlist.
> Did you not reproduce this even with the second/debug patch I've sent?
Nope. (I tried on a sam9g35 with buildroot/busybox)

I have a sama5d3 with a debian as a home server, I'll give it a try.

> 
>> (do you use the console on serial debug ? which board ? )
> up to date debian sid booting with systemd. After boot completed I do
> "cat /proc/interrupts" and check for the "gone" string.
> This is
> [    0.000000] OF: fdt:Machine model: SAMA5D3 Xplained
> [    0.000000] Kernel command line: earlyprintk=ttyS0,115200 console=ttyS0,115200 root=/dev/mmcblk0p2 rootwait
> [    0.520000] AT91: Detected SoC family: sama5d3
> [    0.520000] AT91: Detected SoC: sama5d36, revision 2
> 
> at91-sama5d3_xplained.dtb
> 
> Is this enough?

Great,

Thanks !

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] tty/serial: atmel: use port->name as name in request_irq()
  2018-05-03 13:34             ` Richard Genoud
@ 2018-05-03 15:12               ` Richard Genoud
  -1 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-05-03 15:12 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Rob Herring, Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman,
	linux-serial, Jiri Slaby, tglx, linux-arm-kernel

On 03/05/2018 15:34, Richard Genoud wrote:
> On 03/05/2018 14:44, Sebastian Andrzej Siewior wrote:
>> On 2018-05-03 14:36:03 [+0200], Richard Genoud wrote:
>>> Could resend your patch with Fixes: in the commit message ?
>>
>> will do shortly.
>>
>>> BTW, I didn't manage to reproduce the behavior you describe, could you
>>> give me your .config and describe a little more how you manage to
>>> trigger this bug ?
>> .config sent offlist.
>> Did you not reproduce this even with the second/debug patch I've sent?
> Nope. (I tried on a sam9g35 with buildroot/busybox)
> 
> I have a sama5d3 with a debian as a home server, I'll give it a try.

Ok, I reproduced the use-after-free on a SAMA5D2 and on a SAMA5D3, both
with debian (sid / stretch).
After the patch, no mode funny things in /proc/interrupts.

> 
>>
>>> (do you use the console on serial debug ? which board ? )
>> up to date debian sid booting with systemd. After boot completed I do
>> "cat /proc/interrupts" and check for the "gone" string.
>> This is
>> [    0.000000] OF: fdt:Machine model: SAMA5D3 Xplained
>> [    0.000000] Kernel command line: earlyprintk=ttyS0,115200 console=ttyS0,115200 root=/dev/mmcblk0p2 rootwait
>> [    0.520000] AT91: Detected SoC family: sama5d3
>> [    0.520000] AT91: Detected SoC: sama5d36, revision 2
>>
>> at91-sama5d3_xplained.dtb
>>
>> Is this enough?
> 
> Great,
> 
> Thanks !
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-05-03 15:12               ` Richard Genoud
  0 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-05-03 15:12 UTC (permalink / raw)
  To: linux-arm-kernel

On 03/05/2018 15:34, Richard Genoud wrote:
> On 03/05/2018 14:44, Sebastian Andrzej Siewior wrote:
>> On 2018-05-03 14:36:03 [+0200], Richard Genoud wrote:
>>> Could resend your patch with Fixes: in the commit message ?
>>
>> will do shortly.
>>
>>> BTW, I didn't manage to reproduce the behavior you describe, could you
>>> give me your .config and describe a little more how you manage to
>>> trigger this bug ?
>> .config sent offlist.
>> Did you not reproduce this even with the second/debug patch I've sent?
> Nope. (I tried on a sam9g35 with buildroot/busybox)
> 
> I have a sama5d3 with a debian as a home server, I'll give it a try.

Ok, I reproduced the use-after-free on a SAMA5D2 and on a SAMA5D3, both
with debian (sid / stretch).
After the patch, no mode funny things in /proc/interrupts.

> 
>>
>>> (do you use the console on serial debug ? which board ? )
>> up to date debian sid booting with systemd. After boot completed I do
>> "cat /proc/interrupts" and check for the "gone" string.
>> This is
>> [    0.000000] OF: fdt:Machine model: SAMA5D3 Xplained
>> [    0.000000] Kernel command line: earlyprintk=ttyS0,115200 console=ttyS0,115200 root=/dev/mmcblk0p2 rootwait
>> [    0.520000] AT91: Detected SoC family: sama5d3
>> [    0.520000] AT91: Detected SoC: sama5d36, revision 2
>>
>> at91-sama5d3_xplained.dtb
>>
>> Is this enough?
> 
> Great,
> 
> Thanks !
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] tty/serial: atmel: use port->name as name in request_irq()
  2018-05-03 13:34             ` Richard Genoud
@ 2018-05-04  6:35               ` Richard Genoud
  -1 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-05-04  6:35 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Rob Herring, Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman,
	linux-serial, Jiri Slaby, tglx, linux-arm-kernel

On 03/05/2018 15:34, Richard Genoud wrote:
> On 03/05/2018 14:44, Sebastian Andrzej Siewior wrote:
>> On 2018-05-03 14:36:03 [+0200], Richard Genoud wrote:
>>> Could resend your patch with Fixes: in the commit message ?
>>
>> will do shortly.

Actually, this fix can only be applied on stable kernels 4.14+, because
uart_port->name was introduced in 4.12 by commit f7048b15900f ("tty:
serial_core: Add name field to uart_port struct")


>>
>>> BTW, I didn't manage to reproduce the behavior you describe, could you
>>> give me your .config and describe a little more how you manage to
>>> trigger this bug ?
>> .config sent offlist.
>> Did you not reproduce this even with the second/debug patch I've sent?
> Nope. (I tried on a sam9g35 with buildroot/busybox)
> 
> I have a sama5d3 with a debian as a home server, I'll give it a try.
> 
>>
>>> (do you use the console on serial debug ? which board ? )
>> up to date debian sid booting with systemd. After boot completed I do
>> "cat /proc/interrupts" and check for the "gone" string.
>> This is
>> [    0.000000] OF: fdt:Machine model: SAMA5D3 Xplained
>> [    0.000000] Kernel command line: earlyprintk=ttyS0,115200 console=ttyS0,115200 root=/dev/mmcblk0p2 rootwait
>> [    0.520000] AT91: Detected SoC family: sama5d3
>> [    0.520000] AT91: Detected SoC: sama5d36, revision 2
>>
>> at91-sama5d3_xplained.dtb
>>
>> Is this enough?
> 
> Great,
> 
> Thanks !
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-05-04  6:35               ` Richard Genoud
  0 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-05-04  6:35 UTC (permalink / raw)
  To: linux-arm-kernel

On 03/05/2018 15:34, Richard Genoud wrote:
> On 03/05/2018 14:44, Sebastian Andrzej Siewior wrote:
>> On 2018-05-03 14:36:03 [+0200], Richard Genoud wrote:
>>> Could resend your patch with Fixes: in the commit message ?
>>
>> will do shortly.

Actually, this fix can only be applied on stable kernels 4.14+, because
uart_port->name was introduced in 4.12 by commit f7048b15900f ("tty:
serial_core: Add name field to uart_port struct")


>>
>>> BTW, I didn't manage to reproduce the behavior you describe, could you
>>> give me your .config and describe a little more how you manage to
>>> trigger this bug ?
>> .config sent offlist.
>> Did you not reproduce this even with the second/debug patch I've sent?
> Nope. (I tried on a sam9g35 with buildroot/busybox)
> 
> I have a sama5d3 with a debian as a home server, I'll give it a try.
> 
>>
>>> (do you use the console on serial debug ? which board ? )
>> up to date debian sid booting with systemd. After boot completed I do
>> "cat /proc/interrupts" and check for the "gone" string.
>> This is
>> [    0.000000] OF: fdt:Machine model: SAMA5D3 Xplained
>> [    0.000000] Kernel command line: earlyprintk=ttyS0,115200 console=ttyS0,115200 root=/dev/mmcblk0p2 rootwait
>> [    0.520000] AT91: Detected SoC family: sama5d3
>> [    0.520000] AT91: Detected SoC: sama5d36, revision 2
>>
>> at91-sama5d3_xplained.dtb
>>
>> Is this enough?
> 
> Great,
> 
> Thanks !
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2] tty/serial: atmel: use port->name as name in request_irq()
  2018-05-04  6:35               ` Richard Genoud
@ 2018-05-04  8:14                 ` Sebastian Andrzej Siewior
  -1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-04  8:14 UTC (permalink / raw)
  To: Richard Genoud
  Cc: Rob Herring, Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman,
	linux-serial, Jiri Slaby, tglx, linux-arm-kernel

I was puzzled while looking at /proc/interrupts and random things showed
up between reboots. This occurred more often but I realised it later. The
"correct" output should be:
|38:      11861  atmel-aic5   2 Level     ttyS0

but I saw sometimes
|38:       6426  atmel-aic5   2 Level     tty1

and accounted it wrongly as correct. This is use after free and the
former example randomly got the "old" pointer which pointed to the same
content. With SLAB_FREELIST_RANDOM and HARDENED I even got
|38:       7067  atmel-aic5   2 Level     E=Started User Manager for UID 0

or other nonsense.
As it turns out the tty, pointer that is accessed in atmel_startup(), is
freed() before atmel_shutdown(). It seems to happen quite often that the
tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
don't do anything special - just a systemd boot :)

Use port->name as the IRQ name for request_irq(). This exists as long as
the driver is loaded so no use-after-free here.
For backports before v4.12 I suggest to use `"atmel_serial"' instead
`port->name' (that member was introduced in f7048b15900f ("tty: serial_core:
Add name field to uart_port struct").

Cc: stable@vger.kernel.org
Fixes: 761ed4a94582 ("tty: serial_core: convert uart_close to use tty_port_close")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: - Bisected and added a Fixes tag
       - added a note for backporters to v4.9 … v4.12 (pointed out by
	 Richard Genoud)

 drivers/tty/serial/atmel_serial.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e287fe8f10fc..d3189816740e 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1757,7 +1757,6 @@ static int atmel_startup(struct uart_port *port)
 {
 	struct platform_device *pdev = to_platform_device(port->dev);
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
-	struct tty_struct *tty = port->state->port.tty;
 	int retval;
 
 	/*
@@ -1772,8 +1771,7 @@ static int atmel_startup(struct uart_port *port)
 	 * Allocate the IRQ
 	 */
 	retval = request_irq(port->irq, atmel_interrupt,
-			IRQF_SHARED | IRQF_COND_SUSPEND,
-			tty ? tty->name : "atmel_serial", port);
+			     IRQF_SHARED | IRQF_COND_SUSPEND, port->name, port);
 	if (retval) {
 		dev_err(port->dev, "atmel_startup - Can't get irq\n");
 		return retval;
-- 
2.17.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v2] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-05-04  8:14                 ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-04  8:14 UTC (permalink / raw)
  To: linux-arm-kernel

I was puzzled while looking at /proc/interrupts and random things showed
up between reboots. This occurred more often but I realised it later. The
"correct" output should be:
|38:      11861  atmel-aic5   2 Level     ttyS0

but I saw sometimes
|38:       6426  atmel-aic5   2 Level     tty1

and accounted it wrongly as correct. This is use after free and the
former example randomly got the "old" pointer which pointed to the same
content. With SLAB_FREELIST_RANDOM and HARDENED I even got
|38:       7067  atmel-aic5   2 Level     E=Started User Manager for UID 0

or other nonsense.
As it turns out the tty, pointer that is accessed in atmel_startup(), is
freed() before atmel_shutdown(). It seems to happen quite often that the
tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
don't do anything special - just a systemd boot :)

Use port->name as the IRQ name for request_irq(). This exists as long as
the driver is loaded so no use-after-free here.
For backports before v4.12 I suggest to use `"atmel_serial"' instead
`port->name' (that member was introduced in f7048b15900f ("tty: serial_core:
Add name field to uart_port struct").

Cc: stable at vger.kernel.org
Fixes: 761ed4a94582 ("tty: serial_core: convert uart_close to use tty_port_close")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1?v2: - Bisected and added a Fixes tag
       - added a note for backporters to v4.9 ? v4.12 (pointed out by
	 Richard Genoud)

 drivers/tty/serial/atmel_serial.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e287fe8f10fc..d3189816740e 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1757,7 +1757,6 @@ static int atmel_startup(struct uart_port *port)
 {
 	struct platform_device *pdev = to_platform_device(port->dev);
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
-	struct tty_struct *tty = port->state->port.tty;
 	int retval;
 
 	/*
@@ -1772,8 +1771,7 @@ static int atmel_startup(struct uart_port *port)
 	 * Allocate the IRQ
 	 */
 	retval = request_irq(port->irq, atmel_interrupt,
-			IRQF_SHARED | IRQF_COND_SUSPEND,
-			tty ? tty->name : "atmel_serial", port);
+			     IRQF_SHARED | IRQF_COND_SUSPEND, port->name, port);
 	if (retval) {
 		dev_err(port->dev, "atmel_startup - Can't get irq\n");
 		return retval;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH v2] tty/serial: atmel: use port->name as name in request_irq()
  2018-05-04  8:14                 ` Sebastian Andrzej Siewior
@ 2018-05-04 10:28                   ` Richard Genoud
  -1 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-05-04 10:28 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Rob Herring, Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman,
	linux-serial, Jiri Slaby, tglx, linux-arm-kernel

On 04/05/2018 10:14, Sebastian Andrzej Siewior wrote:
> I was puzzled while looking at /proc/interrupts and random things showed
> up between reboots. This occurred more often but I realised it later. The
> "correct" output should be:
> |38:      11861  atmel-aic5   2 Level     ttyS0
> 
> but I saw sometimes
> |38:       6426  atmel-aic5   2 Level     tty1
> 
> and accounted it wrongly as correct. This is use after free and the
> former example randomly got the "old" pointer which pointed to the same
> content. With SLAB_FREELIST_RANDOM and HARDENED I even got
> |38:       7067  atmel-aic5   2 Level     E=Started User Manager for UID 0
> 
> or other nonsense.
> As it turns out the tty, pointer that is accessed in atmel_startup(), is
> freed() before atmel_shutdown(). It seems to happen quite often that the
> tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
> don't do anything special - just a systemd boot :)
> 
> Use port->name as the IRQ name for request_irq(). This exists as long as
> the driver is loaded so no use-after-free here.
> For backports before v4.12 I suggest to use `"atmel_serial"' instead
> `port->name' (that member was introduced in f7048b15900f ("tty: serial_core:
> Add name field to uart_port struct").
> 
> Cc: stable@vger.kernel.org
I think it's safer to use:
Cc: stable@vger.kernel.org # 4.14
Because the stable team may miss your comment, and even if they don't, I
think it's not their role to adapt the patch to 4.9.x (and test it !)
IMHO, the best way is to add # 4.14 and when it's applied on 4.14.x,
send a tested backport for 4.9.x

Besides that, you can add my:
Acked-by: Richard Genoud <richard.genoud@gmail.com>

Rob, do you agree with this fix ?


> Fixes: 761ed4a94582 ("tty: serial_core: convert uart_close to use tty_port_close")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1…v2: - Bisected and added a Fixes tag
>        - added a note for backporters to v4.9 … v4.12 (pointed out by
> 	 Richard Genoud)
> 
>  drivers/tty/serial/atmel_serial.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index e287fe8f10fc..d3189816740e 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -1757,7 +1757,6 @@ static int atmel_startup(struct uart_port *port)
>  {
>  	struct platform_device *pdev = to_platform_device(port->dev);
>  	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> -	struct tty_struct *tty = port->state->port.tty;
>  	int retval;
>  
>  	/*
> @@ -1772,8 +1771,7 @@ static int atmel_startup(struct uart_port *port)
>  	 * Allocate the IRQ
>  	 */
>  	retval = request_irq(port->irq, atmel_interrupt,
> -			IRQF_SHARED | IRQF_COND_SUSPEND,
> -			tty ? tty->name : "atmel_serial", port);
> +			     IRQF_SHARED | IRQF_COND_SUSPEND, port->name, port);
>  	if (retval) {
>  		dev_err(port->dev, "atmel_startup - Can't get irq\n");
>  		return retval;
> 

Thanks !

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-05-04 10:28                   ` Richard Genoud
  0 siblings, 0 replies; 26+ messages in thread
From: Richard Genoud @ 2018-05-04 10:28 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/05/2018 10:14, Sebastian Andrzej Siewior wrote:
> I was puzzled while looking at /proc/interrupts and random things showed
> up between reboots. This occurred more often but I realised it later. The
> "correct" output should be:
> |38:      11861  atmel-aic5   2 Level     ttyS0
> 
> but I saw sometimes
> |38:       6426  atmel-aic5   2 Level     tty1
> 
> and accounted it wrongly as correct. This is use after free and the
> former example randomly got the "old" pointer which pointed to the same
> content. With SLAB_FREELIST_RANDOM and HARDENED I even got
> |38:       7067  atmel-aic5   2 Level     E=Started User Manager for UID 0
> 
> or other nonsense.
> As it turns out the tty, pointer that is accessed in atmel_startup(), is
> freed() before atmel_shutdown(). It seems to happen quite often that the
> tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
> don't do anything special - just a systemd boot :)
> 
> Use port->name as the IRQ name for request_irq(). This exists as long as
> the driver is loaded so no use-after-free here.
> For backports before v4.12 I suggest to use `"atmel_serial"' instead
> `port->name' (that member was introduced in f7048b15900f ("tty: serial_core:
> Add name field to uart_port struct").
> 
> Cc: stable at vger.kernel.org
I think it's safer to use:
Cc: stable at vger.kernel.org # 4.14
Because the stable team may miss your comment, and even if they don't, I
think it's not their role to adapt the patch to 4.9.x (and test it !)
IMHO, the best way is to add # 4.14 and when it's applied on 4.14.x,
send a tested backport for 4.9.x

Besides that, you can add my:
Acked-by: Richard Genoud <richard.genoud@gmail.com>

Rob, do you agree with this fix ?


> Fixes: 761ed4a94582 ("tty: serial_core: convert uart_close to use tty_port_close")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1?v2: - Bisected and added a Fixes tag
>        - added a note for backporters to v4.9 ? v4.12 (pointed out by
> 	 Richard Genoud)
> 
>  drivers/tty/serial/atmel_serial.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index e287fe8f10fc..d3189816740e 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -1757,7 +1757,6 @@ static int atmel_startup(struct uart_port *port)
>  {
>  	struct platform_device *pdev = to_platform_device(port->dev);
>  	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> -	struct tty_struct *tty = port->state->port.tty;
>  	int retval;
>  
>  	/*
> @@ -1772,8 +1771,7 @@ static int atmel_startup(struct uart_port *port)
>  	 * Allocate the IRQ
>  	 */
>  	retval = request_irq(port->irq, atmel_interrupt,
> -			IRQF_SHARED | IRQF_COND_SUSPEND,
> -			tty ? tty->name : "atmel_serial", port);
> +			     IRQF_SHARED | IRQF_COND_SUSPEND, port->name, port);
>  	if (retval) {
>  		dev_err(port->dev, "atmel_startup - Can't get irq\n");
>  		return retval;
> 

Thanks !

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2] tty/serial: atmel: use port->name as name in request_irq()
  2018-05-04 10:28                   ` Richard Genoud
@ 2018-05-04 20:23                     ` Rob Herring
  -1 siblings, 0 replies; 26+ messages in thread
From: Rob Herring @ 2018-05-04 20:23 UTC (permalink / raw)
  To: Richard Genoud
  Cc: Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman,
	Sebastian Andrzej Siewior, open list:SERIAL DRIVERS, Jiri Slaby,
	Thomas Gleixner,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On Fri, May 4, 2018 at 5:28 AM, Richard Genoud <richard.genoud@gmail.com> wrote:
> On 04/05/2018 10:14, Sebastian Andrzej Siewior wrote:
>> I was puzzled while looking at /proc/interrupts and random things showed
>> up between reboots. This occurred more often but I realised it later. The
>> "correct" output should be:
>> |38:      11861  atmel-aic5   2 Level     ttyS0
>>
>> but I saw sometimes
>> |38:       6426  atmel-aic5   2 Level     tty1
>>
>> and accounted it wrongly as correct. This is use after free and the
>> former example randomly got the "old" pointer which pointed to the same
>> content. With SLAB_FREELIST_RANDOM and HARDENED I even got
>> |38:       7067  atmel-aic5   2 Level     E=Started User Manager for UID 0
>>
>> or other nonsense.
>> As it turns out the tty, pointer that is accessed in atmel_startup(), is
>> freed() before atmel_shutdown(). It seems to happen quite often that the
>> tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
>> don't do anything special - just a systemd boot :)
>>
>> Use port->name as the IRQ name for request_irq(). This exists as long as
>> the driver is loaded so no use-after-free here.
>> For backports before v4.12 I suggest to use `"atmel_serial"' instead
>> `port->name' (that member was introduced in f7048b15900f ("tty: serial_core:
>> Add name field to uart_port struct").
>>
>> Cc: stable@vger.kernel.org
> I think it's safer to use:
> Cc: stable@vger.kernel.org # 4.14
> Because the stable team may miss your comment, and even if they don't, I
> think it's not their role to adapt the patch to 4.9.x (and test it !)
> IMHO, the best way is to add # 4.14 and when it's applied on 4.14.x,
> send a tested backport for 4.9.x
>
> Besides that, you can add my:
> Acked-by: Richard Genoud <richard.genoud@gmail.com>
>
> Rob, do you agree with this fix ?

Yes, one less dependency on tty struct in serial drivers is a good
thing. However, I find port->name to be a somewhat pointless addition.
Most platform drivers (which most serial drivers are) use the device
name for registering their IRQ. And now serial drivers may not even
have a tty with serdev. Using the device name would also solve your
backporting issue. But either way:

Acked-by: Rob Herring <robh@kernel.org>

Rob

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-05-04 20:23                     ` Rob Herring
  0 siblings, 0 replies; 26+ messages in thread
From: Rob Herring @ 2018-05-04 20:23 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, May 4, 2018 at 5:28 AM, Richard Genoud <richard.genoud@gmail.com> wrote:
> On 04/05/2018 10:14, Sebastian Andrzej Siewior wrote:
>> I was puzzled while looking at /proc/interrupts and random things showed
>> up between reboots. This occurred more often but I realised it later. The
>> "correct" output should be:
>> |38:      11861  atmel-aic5   2 Level     ttyS0
>>
>> but I saw sometimes
>> |38:       6426  atmel-aic5   2 Level     tty1
>>
>> and accounted it wrongly as correct. This is use after free and the
>> former example randomly got the "old" pointer which pointed to the same
>> content. With SLAB_FREELIST_RANDOM and HARDENED I even got
>> |38:       7067  atmel-aic5   2 Level     E=Started User Manager for UID 0
>>
>> or other nonsense.
>> As it turns out the tty, pointer that is accessed in atmel_startup(), is
>> freed() before atmel_shutdown(). It seems to happen quite often that the
>> tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
>> don't do anything special - just a systemd boot :)
>>
>> Use port->name as the IRQ name for request_irq(). This exists as long as
>> the driver is loaded so no use-after-free here.
>> For backports before v4.12 I suggest to use `"atmel_serial"' instead
>> `port->name' (that member was introduced in f7048b15900f ("tty: serial_core:
>> Add name field to uart_port struct").
>>
>> Cc: stable at vger.kernel.org
> I think it's safer to use:
> Cc: stable at vger.kernel.org # 4.14
> Because the stable team may miss your comment, and even if they don't, I
> think it's not their role to adapt the patch to 4.9.x (and test it !)
> IMHO, the best way is to add # 4.14 and when it's applied on 4.14.x,
> send a tested backport for 4.9.x
>
> Besides that, you can add my:
> Acked-by: Richard Genoud <richard.genoud@gmail.com>
>
> Rob, do you agree with this fix ?

Yes, one less dependency on tty struct in serial drivers is a good
thing. However, I find port->name to be a somewhat pointless addition.
Most platform drivers (which most serial drivers are) use the device
name for registering their IRQ. And now serial drivers may not even
have a tty with serdev. Using the device name would also solve your
backporting issue. But either way:

Acked-by: Rob Herring <robh@kernel.org>

Rob

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3] tty/serial: atmel: use port->name as name in request_irq()
  2018-05-04 20:23                     ` Rob Herring
@ 2018-05-07 17:11                       ` Sebastian Andrzej Siewior
  -1 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-07 17:11 UTC (permalink / raw)
  To: Rob Herring
  Cc: Alexandre Belloni, Peter Hurley, Richard Genoud,
	Greg Kroah-Hartman, open list:SERIAL DRIVERS, Jiri Slaby,
	Thomas Gleixner,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

I was puzzled while looking at /proc/interrupts and random things showed
up between reboots. This occurred more often but I realised it later. The
"correct" output should be:
|38:      11861  atmel-aic5   2 Level     ttyS0

but I saw sometimes
|38:       6426  atmel-aic5   2 Level     tty1

and accounted it wrongly as correct. This is use after free and the
former example randomly got the "old" pointer which pointed to the same
content. With SLAB_FREELIST_RANDOM and HARDENED I even got
|38:       7067  atmel-aic5   2 Level     E=Started User Manager for UID 0

or other nonsense.
As it turns out the tty, pointer that is accessed in atmel_startup(), is
freed() before atmel_shutdown(). It seems to happen quite often that the
tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
don't do anything special - just a systemd boot :)

Use dev_name(&pdev->dev) as the IRQ name for request_irq(). This exists
as long as the driver is loaded so no use-after-free here.

Cc: stable@vger.kernel.org
Fixes: 761ed4a94582 ("tty: serial_core: convert uart_close to use tty_port_close")
Acked-by: Richard Genoud <richard.genoud@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v3: - Add Acked-by
       - replace port->name -> dev_name(&pdev->dev) to ease work for
	 backporters (suggested by Rob)
v1…v2: add Fixes after bisect.

 drivers/tty/serial/atmel_serial.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e287fe8f10fc..55b3eff148b1 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1757,7 +1757,6 @@ static int atmel_startup(struct uart_port *port)
 {
 	struct platform_device *pdev = to_platform_device(port->dev);
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
-	struct tty_struct *tty = port->state->port.tty;
 	int retval;
 
 	/*
@@ -1772,8 +1771,8 @@ static int atmel_startup(struct uart_port *port)
 	 * Allocate the IRQ
 	 */
 	retval = request_irq(port->irq, atmel_interrupt,
-			IRQF_SHARED | IRQF_COND_SUSPEND,
-			tty ? tty->name : "atmel_serial", port);
+			     IRQF_SHARED | IRQF_COND_SUSPEND,
+			     dev_name(&pdev->dev), port);
 	if (retval) {
 		dev_err(port->dev, "atmel_startup - Can't get irq\n");
 		return retval;
-- 
2.17.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3] tty/serial: atmel: use port->name as name in request_irq()
@ 2018-05-07 17:11                       ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-07 17:11 UTC (permalink / raw)
  To: linux-arm-kernel

I was puzzled while looking at /proc/interrupts and random things showed
up between reboots. This occurred more often but I realised it later. The
"correct" output should be:
|38:      11861  atmel-aic5   2 Level     ttyS0

but I saw sometimes
|38:       6426  atmel-aic5   2 Level     tty1

and accounted it wrongly as correct. This is use after free and the
former example randomly got the "old" pointer which pointed to the same
content. With SLAB_FREELIST_RANDOM and HARDENED I even got
|38:       7067  atmel-aic5   2 Level     E=Started User Manager for UID 0

or other nonsense.
As it turns out the tty, pointer that is accessed in atmel_startup(), is
freed() before atmel_shutdown(). It seems to happen quite often that the
tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
don't do anything special - just a systemd boot :)

Use dev_name(&pdev->dev) as the IRQ name for request_irq(). This exists
as long as the driver is loaded so no use-after-free here.

Cc: stable at vger.kernel.org
Fixes: 761ed4a94582 ("tty: serial_core: convert uart_close to use tty_port_close")
Acked-by: Richard Genoud <richard.genoud@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2?v3: - Add Acked-by
       - replace port->name -> dev_name(&pdev->dev) to ease work for
	 backporters (suggested by Rob)
v1?v2: add Fixes after bisect.

 drivers/tty/serial/atmel_serial.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e287fe8f10fc..55b3eff148b1 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1757,7 +1757,6 @@ static int atmel_startup(struct uart_port *port)
 {
 	struct platform_device *pdev = to_platform_device(port->dev);
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
-	struct tty_struct *tty = port->state->port.tty;
 	int retval;
 
 	/*
@@ -1772,8 +1771,8 @@ static int atmel_startup(struct uart_port *port)
 	 * Allocate the IRQ
 	 */
 	retval = request_irq(port->irq, atmel_interrupt,
-			IRQF_SHARED | IRQF_COND_SUSPEND,
-			tty ? tty->name : "atmel_serial", port);
+			     IRQF_SHARED | IRQF_COND_SUSPEND,
+			     dev_name(&pdev->dev), port);
 	if (retval) {
 		dev_err(port->dev, "atmel_startup - Can't get irq\n");
 		return retval;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2018-05-07 17:11 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-26 15:06 [PATCH] tty/serial: atmel: use port->name as name in request_irq() Sebastian Andrzej Siewior
2018-04-26 15:06 ` Sebastian Andrzej Siewior
2018-04-26 15:12 ` Sebastian Andrzej Siewior
2018-04-26 15:12   ` Sebastian Andrzej Siewior
2018-04-27 10:31   ` Richard Genoud
2018-04-27 10:31     ` Richard Genoud
2018-05-02 19:16     ` Sebastian Andrzej Siewior
2018-05-02 19:16       ` Sebastian Andrzej Siewior
2018-05-03 12:36       ` Richard Genoud
2018-05-03 12:36         ` Richard Genoud
2018-05-03 12:44         ` Sebastian Andrzej Siewior
2018-05-03 12:44           ` Sebastian Andrzej Siewior
2018-05-03 13:34           ` Richard Genoud
2018-05-03 13:34             ` Richard Genoud
2018-05-03 15:12             ` Richard Genoud
2018-05-03 15:12               ` Richard Genoud
2018-05-04  6:35             ` Richard Genoud
2018-05-04  6:35               ` Richard Genoud
2018-05-04  8:14               ` [PATCH v2] " Sebastian Andrzej Siewior
2018-05-04  8:14                 ` Sebastian Andrzej Siewior
2018-05-04 10:28                 ` Richard Genoud
2018-05-04 10:28                   ` Richard Genoud
2018-05-04 20:23                   ` Rob Herring
2018-05-04 20:23                     ` Rob Herring
2018-05-07 17:11                     ` [PATCH v3] " Sebastian Andrzej Siewior
2018-05-07 17:11                       ` Sebastian Andrzej Siewior

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.