| Message ID | 20201227092600.6000-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Commit | 4908067b8f87ebaa9a26514dfe5a9ffba13deb2c |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] dma: bcm6348: incorrect buffer allocation | expand |
Hello Heinrich, This is not swapped. busy_desc is only used in RX. Please, check the rest of the driver: https://github.com/u-boot/u-boot/blob/master/drivers/dma/bcm6348-iudma.c Regards, Álvaro. El 27/12/2020 a las 10:26, Heinrich Schuchardt escribió: > Calling calloc() for 0 members does not make any sense. > Setting ch_priv->busy_desc = NULL for ch_priv->desc_cnt > 0 is equally > unreasonable. > > The current code will lead to a NULL dereference in bcm6348_iudma_enable(). > > The assignments for ch_priv->busy_desc are obviously swapped. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > I have no device to actually test the driver. > --- > drivers/dma/bcm6348-iudma.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/bcm6348-iudma.c b/drivers/dma/bcm6348-iudma.c > index 91172d483c..81597c56a7 100644 > --- a/drivers/dma/bcm6348-iudma.c > +++ b/drivers/dma/bcm6348-iudma.c > @@ -313,10 +313,10 @@ static int bcm6348_iudma_request(struct dma *dma) > ch_priv->desc_id = 0; > if (bcm6348_iudma_chan_is_rx(dma->id)) { > ch_priv->desc_cnt = 0; > - ch_priv->busy_desc = calloc(ch_priv->desc_cnt, sizeof(bool)); > + ch_priv->busy_desc = NULL; > } else { > ch_priv->desc_cnt = ch_priv->dma_ring_size; > - ch_priv->busy_desc = NULL; > + ch_priv->busy_desc = calloc(ch_priv->desc_cnt, sizeof(bool)); > } > > return 0; > -- > 2.29.2 >
On 12/27/20 11:18 AM, Álvaro Fernández Rojas wrote: > Hello Heinrich, > > This is not swapped. > busy_desc is only used in RX. Please, check the rest of the driver: > https://github.com/u-boot/u-boot/blob/master/drivers/dma/bcm6348-iudma.c > > Regards, > Álvaro. Thanks for reviewing. Why is calloc called with count = 0? Couldn't we as well assign NULL? Best regards Heinrich > > El 27/12/2020 a las 10:26, Heinrich Schuchardt escribió: >> Calling calloc() for 0 members does not make any sense. >> Setting ch_priv->busy_desc = NULL for ch_priv->desc_cnt > 0 is equally >> unreasonable. >> >> The current code will lead to a NULL dereference in >> bcm6348_iudma_enable(). >> >> The assignments for ch_priv->busy_desc are obviously swapped. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> I have no device to actually test the driver. >> --- >> drivers/dma/bcm6348-iudma.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/dma/bcm6348-iudma.c b/drivers/dma/bcm6348-iudma.c >> index 91172d483c..81597c56a7 100644 >> --- a/drivers/dma/bcm6348-iudma.c >> +++ b/drivers/dma/bcm6348-iudma.c >> @@ -313,10 +313,10 @@ static int bcm6348_iudma_request(struct dma *dma) >> ch_priv->desc_id = 0; >> if (bcm6348_iudma_chan_is_rx(dma->id)) { >> ch_priv->desc_cnt = 0; >> - ch_priv->busy_desc = calloc(ch_priv->desc_cnt, sizeof(bool)); >> + ch_priv->busy_desc = NULL; >> } else { >> ch_priv->desc_cnt = ch_priv->dma_ring_size; >> - ch_priv->busy_desc = NULL; >> + ch_priv->busy_desc = calloc(ch_priv->desc_cnt, sizeof(bool)); >> } >> >> return 0; >> -- >> 2.29.2 >>
On Sun, Dec 27, 2020 at 10:26:00AM +0100, Heinrich Schuchardt wrote: > Calling calloc() for 0 members does not make any sense. > Setting ch_priv->busy_desc = NULL for ch_priv->desc_cnt > 0 is equally > unreasonable. > > The current code will lead to a NULL dereference in bcm6348_iudma_enable(). > > The assignments for ch_priv->busy_desc are obviously swapped. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> Applied to u-boot/master, thanks!
diff --git a/drivers/dma/bcm6348-iudma.c b/drivers/dma/bcm6348-iudma.c index 91172d483c..81597c56a7 100644 --- a/drivers/dma/bcm6348-iudma.c +++ b/drivers/dma/bcm6348-iudma.c @@ -313,10 +313,10 @@ static int bcm6348_iudma_request(struct dma *dma) ch_priv->desc_id = 0; if (bcm6348_iudma_chan_is_rx(dma->id)) { ch_priv->desc_cnt = 0; - ch_priv->busy_desc = calloc(ch_priv->desc_cnt, sizeof(bool)); + ch_priv->busy_desc = NULL; } else { ch_priv->desc_cnt = ch_priv->dma_ring_size; - ch_priv->busy_desc = NULL; + ch_priv->busy_desc = calloc(ch_priv->desc_cnt, sizeof(bool)); } return 0;
Calling calloc() for 0 members does not make any sense. Setting ch_priv->busy_desc = NULL for ch_priv->desc_cnt > 0 is equally unreasonable. The current code will lead to a NULL dereference in bcm6348_iudma_enable(). The assignments for ch_priv->busy_desc are obviously swapped. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- I have no device to actually test the driver. --- drivers/dma/bcm6348-iudma.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.29.2