* [PATCH] dmaengine: rcar-dmac: avoid array overflow
@ 2017-07-28 13:15 Arnd Bergmann
2017-07-28 16:08 ` Niklas Söderlund
2017-07-29 20:19 ` Laurent Pinchart
0 siblings, 2 replies; 3+ messages in thread
From: Arnd Bergmann @ 2017-07-28 13:15 UTC (permalink / raw)
To: Vinod Koul
Cc: Arnd Bergmann, Dan Williams, Niklas Söderlund,
Laurent Pinchart, Geert Uytterhoeven, Kuninori Morimoto,
dmaengine, linux-kernel
Building with CONFIG_UBSAN_SANITIZE_ALL shows this warning:
drivers/dma/sh/rcar-dmac.c: In function 'rcar_dmac_chan_prep_sg':
drivers/dma/sh/rcar-dmac.c:839:29: error: array subscript is above array bounds [-Werror=array-bounds]
desc->chcr = chcr | chcr_ts[desc->xfer_shift];
As the compiler doesn't know what the xfer_size is, it is impossible
to rule out the array overflow here. As we know that xfer_size
can only be within enum dma_slave_buswidth, this will not overflow
for correct users, and adding a range check will handle the
obscure case and shut up the warning.
Fixes: 87244fe5abdf ("dmaengine: rcar-dmac: Add Renesas R-Car Gen2 DMA Controller (DMAC) driver")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/dma/sh/rcar-dmac.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index ffcadca53243..f5b28eb4009e 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -836,7 +836,8 @@ static void rcar_dmac_chan_configure_desc(struct rcar_dmac_chan *chan,
}
desc->xfer_shift = ilog2(xfer_size);
- desc->chcr = chcr | chcr_ts[desc->xfer_shift];
+ if (desc->xfer_shift < ARRAY_SIZE(chcr_ts))
+ desc->chcr = chcr | chcr_ts[desc->xfer_shift];
}
/*
--
2.9.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] dmaengine: rcar-dmac: avoid array overflow
2017-07-28 13:15 [PATCH] dmaengine: rcar-dmac: avoid array overflow Arnd Bergmann
@ 2017-07-28 16:08 ` Niklas Söderlund
2017-07-29 20:19 ` Laurent Pinchart
1 sibling, 0 replies; 3+ messages in thread
From: Niklas Söderlund @ 2017-07-28 16:08 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Vinod Koul, Dan Williams, Laurent Pinchart, Geert Uytterhoeven,
Kuninori Morimoto, dmaengine, linux-kernel
Hi Arnd,
On 2017-07-28 15:15:49 +0200, Arnd Bergmann wrote:
> Building with CONFIG_UBSAN_SANITIZE_ALL shows this warning:
>
> drivers/dma/sh/rcar-dmac.c: In function 'rcar_dmac_chan_prep_sg':
> drivers/dma/sh/rcar-dmac.c:839:29: error: array subscript is above array bounds [-Werror=array-bounds]
> desc->chcr = chcr | chcr_ts[desc->xfer_shift];
>
> As the compiler doesn't know what the xfer_size is, it is impossible
> to rule out the array overflow here. As we know that xfer_size
> can only be within enum dma_slave_buswidth, this will not overflow
> for correct users, and adding a range check will handle the
> obscure case and shut up the warning.
>
> Fixes: 87244fe5abdf ("dmaengine: rcar-dmac: Add Renesas R-Car Gen2 DMA Controller (DMAC) driver")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> drivers/dma/sh/rcar-dmac.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index ffcadca53243..f5b28eb4009e 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -836,7 +836,8 @@ static void rcar_dmac_chan_configure_desc(struct rcar_dmac_chan *chan,
> }
>
> desc->xfer_shift = ilog2(xfer_size);
> - desc->chcr = chcr | chcr_ts[desc->xfer_shift];
> + if (desc->xfer_shift < ARRAY_SIZE(chcr_ts))
> + desc->chcr = chcr | chcr_ts[desc->xfer_shift];
> }
>
> /*
> --
> 2.9.0
>
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] dmaengine: rcar-dmac: avoid array overflow
2017-07-28 13:15 [PATCH] dmaengine: rcar-dmac: avoid array overflow Arnd Bergmann
2017-07-28 16:08 ` Niklas Söderlund
@ 2017-07-29 20:19 ` Laurent Pinchart
1 sibling, 0 replies; 3+ messages in thread
From: Laurent Pinchart @ 2017-07-29 20:19 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Vinod Koul, Dan Williams, Niklas Söderlund,
Geert Uytterhoeven, Kuninori Morimoto, dmaengine, linux-kernel
Hi Arnd,
Thank you for the patch.
On Friday 28 Jul 2017 15:15:49 Arnd Bergmann wrote:
> Building with CONFIG_UBSAN_SANITIZE_ALL shows this warning:
>
> drivers/dma/sh/rcar-dmac.c: In function 'rcar_dmac_chan_prep_sg':
> drivers/dma/sh/rcar-dmac.c:839:29: error: array subscript is above array
> bounds [-Werror=array-bounds] desc->chcr = chcr |
> chcr_ts[desc->xfer_shift];
>
> As the compiler doesn't know what the xfer_size is, it is impossible
> to rule out the array overflow here. As we know that xfer_size
> can only be within enum dma_slave_buswidth, this will not overflow
> for correct users, and adding a range check will handle the
> obscure case and shut up the warning.
>
> Fixes: 87244fe5abdf ("dmaengine: rcar-dmac: Add Renesas R-Car Gen2 DMA
> Controller (DMAC) driver") Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/dma/sh/rcar-dmac.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index ffcadca53243..f5b28eb4009e 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -836,7 +836,8 @@ static void rcar_dmac_chan_configure_desc(struct
> rcar_dmac_chan *chan, }
>
> desc->xfer_shift = ilog2(xfer_size);
> - desc->chcr = chcr | chcr_ts[desc->xfer_shift];
> + if (desc->xfer_shift < ARRAY_SIZE(chcr_ts))
> + desc->chcr = chcr | chcr_ts[desc->xfer_shift];
I think this counts as an invalid warning. As you stated in the commit
message, we know that xfer_shift is within a valid range of values. True, if
the DMA engine API changed to support larger transfer sizes without updating
the driver, we would have a problem. But your patch will silently leave desc-
>chcr unset in that case, which is not good either. We should instead track
back xfer_size to where it gets set (in rcar_dmac_device_config() if I'm not
mistaken, but I haven't checked in details whether other locations need to be
handled too), and return an error there, possibly with a debug or warning
message.
Assuming we want to guard against that problem (which could be argued), that's
in my opinion the right fix. And it won't get rid of the compiler warning I'm
afraid.
> }
>
> /*
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-07-29 20:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-28 13:15 [PATCH] dmaengine: rcar-dmac: avoid array overflow Arnd Bergmann
2017-07-28 16:08 ` Niklas Söderlund
2017-07-29 20:19 ` Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.