lkml.org 
[lkml]   [2023]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] serial: sprd: Assign sprd_port after initialized to avoid wrong access
From


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?

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.

>> 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;
>>> }
>>>

\
 
 \ /
  Last update: 2023-07-12 03:39    [W:0.163 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site