linux-kernel - RE: [PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for serdev framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AS8PR04MB840408DC92F4001AB440353292BF9@AS8PR04MB8404.eurprd04.prod.outlook.com>
Date:   Wed, 15 Mar 2023 09:49:53 +0000
From:   Sherry Sun <sherry.sun@....com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "jirislaby@...nel.org" <jirislaby@...nel.org>,
        "robh@...nel.org" <robh@...nel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for
 serdev framework



> -----Original Message-----
> From: Greg KH <gregkh@...uxfoundation.org>
> Sent: 2023年3月15日 15:40
> To: Sherry Sun <sherry.sun@....com>
> Cc: jirislaby@...nel.org; robh@...nel.org; linux-serial@...r.kernel.org;
> linux-kernel@...r.kernel.org; dl-linux-imx <linux-imx@....com>
> Subject: Re: [PATCH] tty: serdev: serdev-ttyport: set correct tty->dev for
> serdev framework
> 
> On Wed, Mar 15, 2023 at 03:21:43PM +0800, Sherry Sun wrote:
> > ttyport_open() calls tty_init_dev() to initialize a tty device, but
> > tty_get_device() cannot get the correct tty->dev for serdev tty in
> > alloc_tty_struct(), because serdev framework does not set tty_class,
> > so class_find_device_by_devt(tty_class, devt) may always return NULL.
> >
> > For serdev framework, we need to assign the correct ctrl->dev to
> > tty->dev.
> >
> > Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
> > Signed-off-by: Sherry Sun <sherry.sun@....com>
> > ---
> >  drivers/tty/serdev/serdev-ttyport.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/tty/serdev/serdev-ttyport.c
> > b/drivers/tty/serdev/serdev-ttyport.c
> > index d367803e2044..bba37ab90215 100644
> > --- a/drivers/tty/serdev/serdev-ttyport.c
> > +++ b/drivers/tty/serdev/serdev-ttyport.c
> > @@ -112,6 +112,7 @@ static int ttyport_open(struct serdev_controller
> *ctrl)
> >  	tty = tty_init_dev(serport->tty_drv, serport->tty_idx);
> >  	if (IS_ERR(tty))
> >  		return PTR_ERR(tty);
> > +	tty->dev = &ctrl->dev;
> 
> What in-kernel driver needs this change?  How has it not been a problem so
> far?
> 

Hi Greg, I searched the users of tty->dev under serial floder, found the following drivers need it.
drivers/tty/serial/stm32-usart.c:780:                   pm_wakeup_event(tport->tty->dev, 0);
drivers/tty/serial/fsl_lpuart.c:3018:           tty_dev = tty->dev;
drivers/tty/serial/st-asc.c:266:                pm_wakeup_event(tport->tty->dev, 0);

Actually this issue was found when I tested the nxp Bluetooth driver which use serdev framework along with fsl_lpuart.c driver, when system is suspending, the following NULL pointer kernel panic is observed.
This is because lpuart driver will check the device_may_wakeup(tty->dev) to determine if wakeup register bits need to be enabled or not before suspend, it works well the the ldisc tty, but since serdev tty doesn't set correct tty->dev, so here cause the NULL pointer panic.

root@...8ulpevk:~# echo mem > /sys/power/state
[   42.657779] PM: suspend entry (deep)
[   42.664333] Filesystems sync: 0.002 seconds
[   42.717624] Freezing user space processes ... (elapsed 0.001 seconds) done.
[   42.727063] OOM killer disabled.
[   42.730383] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
[   42.753652] fec 29950000.ethernet eth0: Link is Down
[   42.780681] Unable to handle kernel NULL pointer dereference at virtual address 00000000000000dc
[   42.789603] Mem abort info:
[   42.792430]   ESR = 0x0000000096000004
[   42.796242]   EC = 0x25: DABT (current EL), IL = 32 bits
[   42.801661]   SET = 0, FnV = 0
......

> And why are you saving off a reference counted pointer without
> incrementing the reference to the pointer?

Sorry, forgive me I am not clearly understand the requirement here, do you mean we need to add the following changes?
    get_device(&ctrl->dev);
    tty->dev = &ctrl->dev;
    put_device((&ctrl->dev);

And per my understanding, the reference count needs to be increased and decreased from the user side, here we only do a initialization for the tty->dev.

Best Regards
Sherry