Re: [patch] [media] v4l: xilinx: harmless buffer overflow

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dan,

Thank you for the patch.

On Tuesday 21 April 2015 12:31:10 Dan Carpenter wrote:
> My static checker warns that the name of the port can be 15 characters
> when you consider the NUL terminator and that's one more than the 14
> characters in name[].  Maybe it's an off-by-one?
>
> It's unlikely that we hit the limit and even if we do the overflow will
> only affect one of the two bytes of padding so it's harmless.  Still
> let's fix it and also change the sprintf() to snprintf().
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/media/platform/xilinx/xilinx-dma.c
> b/drivers/media/platform/xilinx/xilinx-dma.c index efde88a..98e50e4 100644
> --- a/drivers/media/platform/xilinx/xilinx-dma.c
> +++ b/drivers/media/platform/xilinx/xilinx-dma.c
> @@ -653,7 +653,7 @@ static const struct v4l2_file_operations xvip_dma_fops =
> { int xvip_dma_init(struct xvip_composite_device *xdev, struct xvip_dma
> *dma, enum v4l2_buf_type type, unsigned int port)
>  {
> -	char name[14];
> +	char name[16];

Being pedantic we could use name[15], but it wouldn't make any difference.

>  	int ret;
> 
>  	dma->xdev = xdev;
> @@ -725,7 +725,7 @@ int xvip_dma_init(struct xvip_composite_device *xdev,
> struct xvip_dma *dma, }
> 
>  	/* ... and the DMA channel. */
> -	sprintf(name, "port%u", port);
> +	snprintf(name, sizeof(name), "port%u", port);

Nitpicking again, I'd say that sprintf is enough as we know it won't overflow. 
However, as the sprintf implementation is a wrapper around vsnprintf with size 
set to INT_MAX, using snprintf won't incur any runtime performance penalty.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree.

>  	dma->dma = dma_request_slave_channel(dma->xdev->dev, name);
>  	if (dma->dma == NULL) {
>  		dev_err(dma->xdev->dev, "no VDMA channel found\n");

-- 
Regards,

Laurent Pinchart