Date:   Wed, 12 Jul 2023 09:39:13 +0800
From:   Baolin Wang <baolin.wang@...ux.alibaba.com>
To:     Chunyan Zhang <zhang.lyra@...il.com>
Cc:     Chunyan Zhang <chunyan.zhang@...soc.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-serial@...r.kernel.org, Orson Zhai <orsonzhai@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] serial: sprd: Assign sprd_port after initialized to
 avoid wrong access



On 7/11/2023 10:57 AM, Chunyan Zhang wrote:
> On Mon, 10 Jul 2023 at 17:57, Baolin Wang <baolin.wang@...ux.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@...soc.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;
>>>    }
>>>

Powered by blists - more mailing lists