[patch] [media] coda: improve safety in coda_register_device()

[patch] [media] coda: improve safety in coda_register_device()

[Dan Carpenter]

The "i" variable is used as an offset into both the dev->vfd[] and the
dev->devtype->vdevs[] arrays.  The second array is smaller so we should
use that as a limit instead of ARRAY_SIZE(dev->vfd).  Also the original
check was off by one.

We should use a format string as well in case the ->name has any funny
characters and also to stop static checkers from complaining.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 39330a7..5dd6cae 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1844,10 +1844,11 @@ static int coda_register_device(struct coda_dev *dev, int i)
 {
 	struct video_device *vfd = &dev->vfd[i];
 
-	if (i > ARRAY_SIZE(dev->vfd))
+	if (i >= dev->devtype->num_vdevs)
 		return -EINVAL;
 
-	snprintf(vfd->name, sizeof(vfd->name), dev->devtype->vdevs[i]->name);
+	snprintf(vfd->name, sizeof(vfd->name), "%s",
+		 dev->devtype->vdevs[i]->name);
 	vfd->fops	= &coda_fops;
 	vfd->ioctl_ops	= &coda_ioctl_ops;
 	vfd->release	= video_device_release_empty,

Re: [patch] [media] coda: improve safety in coda_register_device()

[walter harms]

Am 08.01.2015 11:07, schrieb Dan Carpenter:
> The "i" variable is used as an offset into both the dev->vfd[] and the
> dev->devtype->vdevs[] arrays.  The second array is smaller so we should
> use that as a limit instead of ARRAY_SIZE(dev->vfd).  Also the original
> check was off by one.
> 
> We should use a format string as well in case the ->name has any funny
> characters and also to stop static checkers from complaining.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 39330a7..5dd6cae 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -1844,10 +1844,11 @@ static int coda_register_device(struct coda_dev *dev, int i)
>  {
>  	struct video_device *vfd = &dev->vfd[i];
>  
> -	if (i > ARRAY_SIZE(dev->vfd))
> +	if (i >= dev->devtype->num_vdevs)
>  		return -EINVAL;

hi,
 just a minor question. if i can not be trusted, i feel you should move the
 array access:
   struct video_device *vfd = &dev->vfd[i];
 after the check
   i >= dev->devtype->num_vdevs
at least that would improve the readability by not trigger my internal alarm
"check after access"

re,
 wh


> -	snprintf(vfd->name, sizeof(vfd->name), dev->devtype->vdevs[i]->name);
> +	snprintf(vfd->name, sizeof(vfd->name), "%s",
> +		 dev->devtype->vdevs[i]->name);
>  	vfd->fops	= &coda_fops;
>  	vfd->ioctl_ops	= &coda_ioctl_ops;
>  	vfd->release	= video_device_release_empty,
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch] [media] coda: improve safety in coda_register_device()

[Dan Carpenter]

On Thu, Jan 08, 2015 at 12:04:20PM +0100, walter harms wrote:
> > @@ -1844,10 +1844,11 @@ static int coda_register_device(struct coda_dev *dev, int i)
> >  {
> >  	struct video_device *vfd = &dev->vfd[i];
> >  
> > -	if (i > ARRAY_SIZE(dev->vfd))
> > +	if (i >= dev->devtype->num_vdevs)
> >  		return -EINVAL;
> 
> hi,
>  just a minor question. if i can not be trusted, i feel you should move the
>  array access:
>    struct video_device *vfd = &dev->vfd[i];
>  after the check
>    i >= dev->devtype->num_vdevs
> at least that would improve the readability by not trigger my internal alarm
> "check after access"

The "access" is just taking the address, not dereferencing so it's ok.
This kind of code is fairly common and CodingStyle doesn't have an
opinion here so I left it how the original author wrote it.

regards,
dan carpenter