Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752260AbdG2UT3 (ORCPT ); Sat, 29 Jul 2017 16:19:29 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:48999 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752162AbdG2UT2 (ORCPT ); Sat, 29 Jul 2017 16:19:28 -0400 From: Laurent Pinchart To: Arnd Bergmann Cc: Vinod Koul , Dan Williams , Niklas =?ISO-8859-1?Q?S=F6derlund?= , Geert Uytterhoeven , Kuninori Morimoto , dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] dmaengine: rcar-dmac: avoid array overflow Date: Sat, 29 Jul 2017 23:19:37 +0300 Message-ID: <1781490.W8JVuMnr5Q@avalon> User-Agent: KMail/4.14.10 (Linux/4.9.34-gentoo; KDE/4.14.32; x86_64; ; ) In-Reply-To: <20170728131613.122505-1-arnd@arndb.de> References: <20170728131613.122505-1-arnd@arndb.de> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2249 Lines: 58 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 > --- > 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