The return value of dma_alloc_coherent() needs to be checked.
To avoid dereference of null pointer in case of the failure of alloc.
Signed-off-by: Jiasheng Jiang <[email protected]>
---
Changelog:
v2 -> v3
*Change 1. Remove dev_err.
*Change 2. Change the return type of pch_request_dma to int.
*Change 3. Return -ENOMEM when dma_alloc_coherent() failed and 0 the
others.
*Change 4. Check return value of dma_alloc_coherent().
---
drivers/tty/serial/pch_uart.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index f0351e6f0ef6..cfad5592010c 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -698,7 +698,7 @@ static bool filter(struct dma_chan *chan, void *slave)
}
}
-static void pch_request_dma(struct uart_port *port)
+static int pch_request_dma(struct uart_port *port)
{
dma_cap_mask_t mask;
struct dma_chan *chan;
@@ -723,7 +723,7 @@ static void pch_request_dma(struct uart_port *port)
if (!chan) {
dev_err(priv->port.dev, "%s:dma_request_channel FAILS(Tx)\n",
__func__);
- return;
+ return 0;
}
priv->chan_tx = chan;
@@ -739,13 +739,20 @@ static void pch_request_dma(struct uart_port *port)
__func__);
dma_release_channel(priv->chan_tx);
priv->chan_tx = NULL;
- return;
+ return 0;
}
/* Get Consistent memory for DMA */
priv->rx_buf_virt = dma_alloc_coherent(port->dev, port->fifosize,
&priv->rx_buf_dma, GFP_KERNEL);
+ if (!priv->rx_buf_virt) {
+ dma_release_channel(priv->chan_tx);
+ priv->chan_tx = NULL;
+ return -ENOMEM;
+ }
+
priv->chan_rx = chan;
+ return 0;
}
static void pch_dma_rx_complete(void *arg)
@@ -1321,8 +1328,11 @@ static int pch_uart_startup(struct uart_port *port)
if (ret < 0)
return ret;
- if (priv->use_dma)
- pch_request_dma(port);
+ if (priv->use_dma) {
+ ret = pch_request_dma(port);
+ if (ret)
+ return ret;
+ }
priv->start_rx = 1;
pch_uart_hal_enable_interrupt(priv, PCH_UART_HAL_RX_INT |
@@ -1469,6 +1479,7 @@ static int pch_uart_verify_port(struct uart_port *port,
struct serial_struct *serinfo)
{
struct eg20t_port *priv;
+ int ret;
priv = container_of(port, struct eg20t_port, port);
if (serinfo->flags & UPF_LOW_LATENCY) {
@@ -1483,7 +1494,9 @@ static int pch_uart_verify_port(struct uart_port *port,
return -EOPNOTSUPP;
#endif
if (!priv->use_dma) {
- pch_request_dma(port);
+ ret = pch_request_dma(port);
+ if (ret)
+ return ret;
if (priv->chan_rx)
priv->use_dma = 1;
}
--
2.25.1
On Thu, Dec 16, 2021 at 10:14:54PM +0800, Jiasheng Jiang wrote:
> The return value of dma_alloc_coherent() needs to be checked.
> To avoid dereference of null pointer in case of the failure of alloc.
> Signed-off-by: Jiasheng Jiang <[email protected]>
A blank line is always needed before the signed-off-by line.
> ---
> Changelog:
>
> v2 -> v3
>
> *Change 1. Remove dev_err.
> *Change 2. Change the return type of pch_request_dma to int.
> *Change 3. Return -ENOMEM when dma_alloc_coherent() failed and 0 the
> others.
> *Change 4. Check return value of dma_alloc_coherent().
I see v3 here, not v4. Where is v4?
And how did you test this change?
thanks,
greg k-h
On Thu, Dec 16, 2021 at 10:36:35PM +0800, Greg KH wrote:
>> The return value of dma_alloc_coherent() needs to be checked.
>> To avoid dereference of null pointer in case of the failure of alloc.
>> Signed-off-by: Jiasheng Jiang <[email protected]>
>A blank line is always needed before the signed-off-by line.
>> ---
>> Changelog:
>>
>> v2 -> v3
>>
>> *Change 1. Remove dev_err.
>> *Change 2. Change the return type of pch_request_dma to int.
>> *Change 3. Return -ENOMEM when dma_alloc_coherent() failed and 0 the
>> others.
>> *Change 4. Check return value of dma_alloc_coherent().
>
> I see v3 here, not v4. Where is v4?
>
> And how did you test this change?
>
> thanks,
>
> greg k-h
Sorry, I just have v3, maybe that is my fault. But I don't know why you think there is v4.
And I have no idea about the "test this change"? Please give me more detail.
Sincerely thanks,
jiasheng jiang
On Thu, Dec 16, 2021 at 11:05:39PM +0800, Jiasheng Jiang wrote:
> On Thu, Dec 16, 2021 at 10:36:35PM +0800, Greg KH wrote:
> >> The return value of dma_alloc_coherent() needs to be checked.
> >> To avoid dereference of null pointer in case of the failure of alloc.
> >> Signed-off-by: Jiasheng Jiang <[email protected]>
>
> >A blank line is always needed before the signed-off-by line.
>
> >> ---
> >> Changelog:
> >>
> >> v2 -> v3
> >>
> >> *Change 1. Remove dev_err.
> >> *Change 2. Change the return type of pch_request_dma to int.
> >> *Change 3. Return -ENOMEM when dma_alloc_coherent() failed and 0 the
> >> others.
> >> *Change 4. Check return value of dma_alloc_coherent().
> >
> > I see v3 here, not v4. Where is v4?
> >
> > And how did you test this change?
> >
> > thanks,
> >
> > greg k-h
>
> Sorry, I just have v3, maybe that is my fault. But I don't know why you think there is v4.
You say "change 4". Am I confused?
> And I have no idea about the "test this change"? Please give me more detail.
Did you run the kernel before your change and then after your change to
ensure that the failure you saw before your change is now properly
fixed?
How did you test this?
thanks,
greg k-h
On Thu, Dec 16, 2021 at 11:24:36PM +0800, Greg KH wrote:
> Did you run the kernel before your change and then after your change to
> ensure that the failure you saw before your change is now properly
> fixed?
>
> How did you test this?
Thank you for your answer. But I haven't run it yet.
And in fact I don't know how to trigger the alloc failure.
I just find the other place like eni_init_one() in `drivers/atm/eni.c`
check the return value of the dma_alloc_coherent().
So that's possible to alloc failure.
Therefore, in order to avoid the dereference of the null pointer, it might
be better to deal with it.
Sincerely thanks,
jiasheng jiang
On Thu, Dec 16, 2021 at 11:41:13PM +0800, Jiasheng Jiang wrote:
> On Thu, Dec 16, 2021 at 11:24:36PM +0800, Greg KH wrote:
> > Did you run the kernel before your change and then after your change to
> > ensure that the failure you saw before your change is now properly
> > fixed?
> >
> > How did you test this?
>
> Thank you for your answer. But I haven't run it yet.
> And in fact I don't know how to trigger the alloc failure.
Then perhaps this change is not needed?
thanks,
greg k-h
On Fri, Dec 17, 2021 at 00:14:28AM +0800, Greg KH wrote:
>> Thank you for your answer. But I haven't run it yet.
>> And in fact I don't know how to trigger the alloc failure.
>
> Then perhaps this change is not needed?
Er, I mean I have not got the technique to create the sitution that
the alloc would fail.
But it is universally accepted that the alloc would fail when the system
is under an extreme memory pressure.
If you insist to recurrent the bug, I will try my best to do it,
which I think it is needless.
Sincerely thanks,
jiasheng jiang
On Thu, Dec 16, 2021 at 10:14:54PM +0800, Jiasheng Jiang wrote:
> The return value of dma_alloc_coherent() needs to be checked.
> To avoid dereference of null pointer in case of the failure of alloc.
> Signed-off-by: Jiasheng Jiang <[email protected]>
> ---
> Changelog:
>
> v2 -> v3
>
> *Change 1. Remove dev_err.
> *Change 2. Change the return type of pch_request_dma to int.
> *Change 3. Return -ENOMEM when dma_alloc_coherent() failed and 0 the
> others.
> *Change 4. Check return value of dma_alloc_coherent().
> ---
> drivers/tty/serial/pch_uart.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
> index f0351e6f0ef6..cfad5592010c 100644
> --- a/drivers/tty/serial/pch_uart.c
> +++ b/drivers/tty/serial/pch_uart.c
> @@ -698,7 +698,7 @@ static bool filter(struct dma_chan *chan, void *slave)
> }
> }
>
> -static void pch_request_dma(struct uart_port *port)
> +static int pch_request_dma(struct uart_port *port)
> {
> dma_cap_mask_t mask;
> struct dma_chan *chan;
> @@ -723,7 +723,7 @@ static void pch_request_dma(struct uart_port *port)
> if (!chan) {
> dev_err(priv->port.dev, "%s:dma_request_channel FAILS(Tx)\n",
> __func__);
> - return;
> + return 0;
> }
> priv->chan_tx = chan;
>
> @@ -739,13 +739,20 @@ static void pch_request_dma(struct uart_port *port)
> __func__);
> dma_release_channel(priv->chan_tx);
> priv->chan_tx = NULL;
> - return;
> + return 0;
> }
>
> /* Get Consistent memory for DMA */
> priv->rx_buf_virt = dma_alloc_coherent(port->dev, port->fifosize,
> &priv->rx_buf_dma, GFP_KERNEL);
> + if (!priv->rx_buf_virt) {
> + dma_release_channel(priv->chan_tx);
> + priv->chan_tx = NULL;
> + return -ENOMEM;
> + }
> +
> priv->chan_rx = chan;
> + return 0;
> }
>
> static void pch_dma_rx_complete(void *arg)
> @@ -1321,8 +1328,11 @@ static int pch_uart_startup(struct uart_port *port)
> if (ret < 0)
> return ret;
>
> - if (priv->use_dma)
> - pch_request_dma(port);
> + if (priv->use_dma) {
> + ret = pch_request_dma(port);
> + if (ret)
> + return ret;
> + }
>
> priv->start_rx = 1;
> pch_uart_hal_enable_interrupt(priv, PCH_UART_HAL_RX_INT |
> @@ -1469,6 +1479,7 @@ static int pch_uart_verify_port(struct uart_port *port,
> struct serial_struct *serinfo)
> {
> struct eg20t_port *priv;
> + int ret;
>
> priv = container_of(port, struct eg20t_port, port);
> if (serinfo->flags & UPF_LOW_LATENCY) {
> @@ -1483,7 +1494,9 @@ static int pch_uart_verify_port(struct uart_port *port,
> return -EOPNOTSUPP;
> #endif
> if (!priv->use_dma) {
> - pch_request_dma(port);
> + ret = pch_request_dma(port);
> + if (ret)
> + return ret;
> if (priv->chan_rx)
> priv->use_dma = 1;
> }
> --
> 2.25.1
>
This patch is obviously not correct, and will cause problems if it were
accepted.
Please work on whatever tool you are using to find and make these
changes, as it is not working properly.
Or, if this was a manual change, please work on your kernel programming
skills. There are a number of bugs in this proposed change, showing
that it was not tested at all.
thanks,
greg k-h
On Mon, Dec 20, 2021 at 11:37:58AM +0800, Greg KH wrote:
> This patch is obviously not correct, and will cause problems if it were
> accepted.
>
> Please work on whatever tool you are using to find and make these
> changes, as it is not working properly.
>
> Or, if this was a manual change, please work on your kernel programming
> skills. There are a number of bugs in this proposed change, showing
> that it was not tested at all.
Sorry, I am not so familiar with the kernel and I really change the code manually.
Could you please tell me what's wrong with my patch with more details?
And I just find that I may forget to release the chan.
Is that all?
Or there are other bugs I need to deal with.
Sincerely thanks,
Jiasheng