On Wed, Jan 10, 2024 at 10:46:02PM +0000, Lad, Prabhakar wrote: > Hi Kees, > > Thank you for the review. > > On Wed, Jan 10, 2024 at 10:41 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote: > > > > On Wed, Jan 10, 2024 at 10:22:10PM +0000, Prabhakar wrote: > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > > > gcc points out that the fix-byte buffer might be too small: > > > drivers/dma/sh/usb-dmac.c: In function 'usb_dmac_probe': > > > drivers/dma/sh/usb-dmac.c:720:34: warning: '%u' directive writing between 1 and 10 bytes into a region of size 3 [-Wformat-overflow=] > > > 720 | sprintf(pdev_irqname, "ch%u", index); > > > | ^~ > > > In function 'usb_dmac_chan_probe', > > > inlined from 'usb_dmac_probe' at drivers/dma/sh/usb-dmac.c:814:9: > > > drivers/dma/sh/usb-dmac.c:720:31: note: directive argument in the range [0, 4294967294] > > > 720 | sprintf(pdev_irqname, "ch%u", index); > > > | ^~~~~~ > > > drivers/dma/sh/usb-dmac.c:720:9: note: 'sprintf' output between 4 and 13 bytes into a destination of size 5 > > > 720 | sprintf(pdev_irqname, "ch%u", index); > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > Maximum number of channels for USB-DMAC as per the driver is 1-99 so use > > > u8 instead of unsigned int/int for DMAC channel indexing and make the > > > pdev_irqname string long enough to avoid the warning. > > > > > > While at it use scnprintf() instead of sprintf() to make the code more > > > robust. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > > > This looks like good fixes; thanks! I see n_channels is sanity checked > > during the probe in usb_dmac_chan_probe(), so this looks good. > > > > (Is there a reason not to also change n_channels to a u8?) > > > Good point, I oversighted it by just looking at the loop indices. I > will send a v2 with that change. I think you'll need a bounce variable in usb_dmac_chan_probe() since it looks like it's reading a 32-bit value from DT, but otherwise, it should be okay. -Kees -- Kees Cook
