| Message ID | 20230710080348.4137875-1-chunyan.zhang@unisoc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] serial: sprd: Assign sprd_port after initialized to avoid wrong access | expand |
On 7/10/2023 4:03 PM, Chunyan Zhang wrote: > The global pointer 'sprd_port' maybe not zero when sprd_probe returns > fail, that is a risk for sprd_port to be accessed afterward, and will > lead unexpected errors. > > For example: > > There're two UART ports, UART1 is used for console and configured in kernel > command line, i.e. "console="; > > The UART1 probe fail and the memory allocated to sprd_port[1] was released, > but sprd_port[1] was not set to NULL; IMO, we should just set sprd_port[1] to be NULL, which seems simpler? > > In UART2 probe, the same virtual address was allocated to sprd_port[2], > and UART2 probe process finally will go into sprd_console_setup() to > register UART1 as console since it is configured as preferred console > (filled to console_cmdline[]), but the console parameters (sprd_port[1]) > actually belongs to UART2. I'm confusing why the console parameters belongs to UART2? Since the console_cmdline[] will specify the serial index, that belongs to UART1. Please correct me if I miss something. > So move the sprd_port[] assignment to where the port already initialized > can avoid the above issue. > > Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support") > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > --- > drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c > index b58f51296ace..942808517393 100644 > --- a/drivers/tty/serial/sprd_serial.c > +++ b/drivers/tty/serial/sprd_serial.c > @@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport) > static int sprd_clk_init(struct uart_port *uport) > { > struct clk *clk_uart, *clk_parent; > - struct sprd_uart_port *u = sprd_port[uport->line]; > + struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port); > > clk_uart = devm_clk_get(uport->dev, "uart"); > if (IS_ERR(clk_uart)) { > @@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev) > { > struct resource *res; > struct uart_port *up; > + struct sprd_uart_port *sport; > int irq; > int index; > int ret; > > index = of_alias_get_id(pdev->dev.of_node, "serial"); > - if (index < 0 || index >= ARRAY_SIZE(sprd_port)) { > + if (index < 0 || index >= UART_NR_MAX) { > dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index); > return -EINVAL; > } > > - sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]), > - GFP_KERNEL); > - if (!sprd_port[index]) > + sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL); > + if (!sport) > return -ENOMEM; > > - up = &sprd_port[index]->port; > + up = &sport->port; > up->dev = &pdev->dev; > up->line = index; > up->type = PORT_SPRD; > @@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev) > * Allocate one dma buffer to prepare for receive transfer, in case > * memory allocation failure at runtime. > */ > - ret = sprd_rx_alloc_buf(sprd_port[index]); > + ret = sprd_rx_alloc_buf(sport); > if (ret) > return ret; > > @@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev) > } > sprd_ports_num++; > > + sprd_port[index] = sport; > + > ret = uart_add_one_port(&sprd_uart_driver, up); > if (ret) > - sprd_remove(pdev); > + goto clean_port; > > platform_set_drvdata(pdev, up); > > + return 0; > + > +clean_port: > + sprd_port[index] = NULL; > + sprd_ports_num--; > + uart_unregister_driver(&sprd_uart_driver); > return ret; > } >
On Mon, 10 Jul 2023 at 17:57, Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > > > > On 7/10/2023 4:03 PM, Chunyan Zhang wrote: > > The global pointer 'sprd_port' maybe not zero when sprd_probe returns > > fail, that is a risk for sprd_port to be accessed afterward, and will > > lead unexpected errors. > > > > For example: > > > > There're two UART ports, UART1 is used for console and configured in kernel > > command line, i.e. "console="; > > > > The UART1 probe fail and the memory allocated to sprd_port[1] was released, > > but sprd_port[1] was not set to NULL; > > IMO, we should just set sprd_port[1] to be NULL, which seems simpler? This patch just does like this indeed, in the label of 'clean_port'. Adding a local variable instead of using global pointer (sprd_port[]) to store the virtual address allocated for sprd_port can avoid overmany goto labels. > > > > > In UART2 probe, the same virtual address was allocated to sprd_port[2], > > and UART2 probe process finally will go into sprd_console_setup() to > > register UART1 as console since it is configured as preferred console > > (filled to console_cmdline[]), but the console parameters (sprd_port[1]) > > actually belongs to UART2. > > I'm confusing why the console parameters belongs to UART2? Since the > console_cmdline[] will specify the serial index, that belongs to UART1. The same virtual address stored in sprd_port[1] was reallocated to sprd_port[2] after the UART1 probe returned failure. Thanks for the review, Chunyan > Please correct me if I miss something. > > > So move the sprd_port[] assignment to where the port already initialized > > can avoid the above issue. > > > > Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support") > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > > --- > > drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++-------- > > 1 file changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c > > index b58f51296ace..942808517393 100644 > > --- a/drivers/tty/serial/sprd_serial.c > > +++ b/drivers/tty/serial/sprd_serial.c > > @@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport) > > static int sprd_clk_init(struct uart_port *uport) > > { > > struct clk *clk_uart, *clk_parent; > > - struct sprd_uart_port *u = sprd_port[uport->line]; > > + struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port); > > > > clk_uart = devm_clk_get(uport->dev, "uart"); > > if (IS_ERR(clk_uart)) { > > @@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev) > > { > > struct resource *res; > > struct uart_port *up; > > + struct sprd_uart_port *sport; > > int irq; > > int index; > > int ret; > > > > index = of_alias_get_id(pdev->dev.of_node, "serial"); > > - if (index < 0 || index >= ARRAY_SIZE(sprd_port)) { > > + if (index < 0 || index >= UART_NR_MAX) { > > dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index); > > return -EINVAL; > > } > > > > - sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]), > > - GFP_KERNEL); > > - if (!sprd_port[index]) > > + sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL); > > + if (!sport) > > return -ENOMEM; > > > > - up = &sprd_port[index]->port; > > + up = &sport->port; > > up->dev = &pdev->dev; > > up->line = index; > > up->type = PORT_SPRD; > > @@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev) > > * Allocate one dma buffer to prepare for receive transfer, in case > > * memory allocation failure at runtime. > > */ > > - ret = sprd_rx_alloc_buf(sprd_port[index]); > > + ret = sprd_rx_alloc_buf(sport); > > if (ret) > > return ret; > > > > @@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev) > > } > > sprd_ports_num++; > > > > + sprd_port[index] = sport; > > + > > ret = uart_add_one_port(&sprd_uart_driver, up); > > if (ret) > > - sprd_remove(pdev); > > + goto clean_port; > > > > platform_set_drvdata(pdev, up); > > > > + return 0; > > + > > +clean_port: > > + sprd_port[index] = NULL; > > + sprd_ports_num--; > > + uart_unregister_driver(&sprd_uart_driver); > > return ret; > > } > >
On Wed, 12 Jul 2023 at 09:39, Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > > > > On 7/11/2023 10:57 AM, Chunyan Zhang wrote: > > On Mon, 10 Jul 2023 at 17:57, Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > >> > >> > >> > >> On 7/10/2023 4:03 PM, Chunyan Zhang wrote: > >>> The global pointer 'sprd_port' maybe not zero when sprd_probe returns > >>> fail, that is a risk for sprd_port to be accessed afterward, and will > >>> lead unexpected errors. > >>> > >>> For example: > >>> > >>> There're two UART ports, UART1 is used for console and configured in kernel > >>> command line, i.e. "console="; > >>> > >>> The UART1 probe fail and the memory allocated to sprd_port[1] was released, > >>> but sprd_port[1] was not set to NULL; > >> > >> IMO, we should just set sprd_port[1] to be NULL, which seems simpler? > > > > This patch just does like this indeed, in the label of 'clean_port'. > > Adding a local variable instead of using global pointer (sprd_port[]) > > to store the virtual address allocated for sprd_port can avoid > > overmany goto labels. > > > >> > >>> > >>> In UART2 probe, the same virtual address was allocated to sprd_port[2], > >>> and UART2 probe process finally will go into sprd_console_setup() to > >>> register UART1 as console since it is configured as preferred console > >>> (filled to console_cmdline[]), but the console parameters (sprd_port[1]) > >>> actually belongs to UART2. > >> > >> I'm confusing why the console parameters belongs to UART2? Since the > >> console_cmdline[] will specify the serial index, that belongs to UART1. > > > > The same virtual address stored in sprd_port[1] was reallocated to > > sprd_port[2] after the UART1 probe returned failure. > > > > After more thinking, I understood your case. :) > > But I see a nit in this patch, you added a 'clean_port' label to clear > the resource for the fail-probe-port instead of sprd_remove(), however > sprd_remove() will call sprd_rx_free_buf() to free the DMA buffer > originally. I know the 2nd patch will add it back, but patch 1 is not > git-bisect safe, right? Actually it doesn't make git-bisect unsafe, the driver can work with a memory leak issue which has been there and fixed in another patch. But I can add back sprd_remove() to keep the unrelated code logic the same. Thanks for the review, Chunyan > > So I think you should also add sprd_rx_free_buf() under the 'clean_port' > label in patch 1, then patch 2 moves the sprd_rx_free_buf() to the > correct place. >
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c index b58f51296ace..942808517393 100644 --- a/drivers/tty/serial/sprd_serial.c +++ b/drivers/tty/serial/sprd_serial.c @@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport) static int sprd_clk_init(struct uart_port *uport) { struct clk *clk_uart, *clk_parent; - struct sprd_uart_port *u = sprd_port[uport->line]; + struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port); clk_uart = devm_clk_get(uport->dev, "uart"); if (IS_ERR(clk_uart)) { @@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev) { struct resource *res; struct uart_port *up; + struct sprd_uart_port *sport; int irq; int index; int ret; index = of_alias_get_id(pdev->dev.of_node, "serial"); - if (index < 0 || index >= ARRAY_SIZE(sprd_port)) { + if (index < 0 || index >= UART_NR_MAX) { dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index); return -EINVAL; } - sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]), - GFP_KERNEL); - if (!sprd_port[index]) + sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL); + if (!sport) return -ENOMEM; - up = &sprd_port[index]->port; + up = &sport->port; up->dev = &pdev->dev; up->line = index; up->type = PORT_SPRD; @@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev) * Allocate one dma buffer to prepare for receive transfer, in case * memory allocation failure at runtime. */ - ret = sprd_rx_alloc_buf(sprd_port[index]); + ret = sprd_rx_alloc_buf(sport); if (ret) return ret; @@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev) } sprd_ports_num++; + sprd_port[index] = sport; + ret = uart_add_one_port(&sprd_uart_driver, up); if (ret) - sprd_remove(pdev); + goto clean_port; platform_set_drvdata(pdev, up); + return 0; + +clean_port: + sprd_port[index] = NULL; + sprd_ports_num--; + uart_unregister_driver(&sprd_uart_driver); return ret; }
The global pointer 'sprd_port' maybe not zero when sprd_probe returns fail, that is a risk for sprd_port to be accessed afterward, and will lead unexpected errors. For example: There're two UART ports, UART1 is used for console and configured in kernel command line, i.e. "console="; The UART1 probe fail and the memory allocated to sprd_port[1] was released, but sprd_port[1] was not set to NULL; In UART2 probe, the same virtual address was allocated to sprd_port[2], and UART2 probe process finally will go into sprd_console_setup() to register UART1 as console since it is configured as preferred console (filled to console_cmdline[]), but the console parameters (sprd_port[1]) actually belongs to UART2. So move the sprd_port[] assignment to where the port already initialized can avoid the above issue. Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support") Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> --- drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)