[patch] tvaudio: remove bogus check

[patch] tvaudio: remove bogus check

[Vitaly Wool]

Hello Mauro,

below is the patch that removes the subaddr check against ARRAY_SIZE(chip->shadow.bytes).
Regardless of anything, the 'bytes' array is 64 bytes large so this check disables 
easy standard programming (TDA9874A_ESP) which has a number of 255 which is hardly the
intended behavior.

As a matter of fact, we can think of separate check for this case like
	if (subaddr + 1 >= ARRAY_SIZE(chip->shadow.bytes) ||
	    subaddr != 0xFF) {
		... /* weird register, refuse */
	}
but I'm not sure if there are no other special cases so for now I suggest to just disable
it.

 drivers/media/video/tvaudio.c |   17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

Signed-off-by: Vitaly Wool <vital@embeddedalley.com> 

Index: linux-next/drivers/media/video/tvaudio.c
===================================================================
--- linux-next.orig/drivers/media/video/tvaudio.c	2009-03-02 17:50:40.000000000 +0300
+++ linux-next/drivers/media/video/tvaudio.c	2009-03-02 18:08:08.000000000 +0300
@@ -169,13 +169,6 @@
 			return -1;
 		}
 	} else {
-		if (subaddr + 1 >= ARRAY_SIZE(chip->shadow.bytes)) {
-			v4l2_info(sd,
-				"Tried to access a non-existent register: %d\n",
-				subaddr);
-			return -EINVAL;
-		}
-
 		v4l2_dbg(1, debug, sd, "chip_write: reg%d=0x%x\n",
 			subaddr, val);
 		chip->shadow.bytes[subaddr+1] = val;
@@ -198,16 +191,8 @@
 	if (mask != 0) {
 		if (subaddr < 0) {
 			val = (chip->shadow.bytes[1] & ~mask) | (val & mask);
-		} else {
-			if (subaddr + 1 >= ARRAY_SIZE(chip->shadow.bytes)) {
-				v4l2_info(sd,
-					"Tried to access a non-existent register: %d\n",
-					subaddr);
-				return -EINVAL;
-			}
-
+		} else
 			val = (chip->shadow.bytes[subaddr+1] & ~mask) | (val & mask);
-		}
 	}
 	return chip_write(chip, subaddr, val);
 }


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [patch] tvaudio: remove bogus check

[Alexey Klimov]

Hello, Vitaly

Now the main development maillist of V4L is linux-media@vger.kernel.org.
Posting to that maillist also helps patchwork.kernel.org to handle
patches correctly.

Mauro, what is the status of video4linux-list@redhat.com? Is that user
maillist, or is it deprecated mail that should be disabled ? 

On Mon, 2009-03-02 at 18:12 +0300, Vitaly Wool wrote:
> Hello Mauro,
> 
> below is the patch that removes the subaddr check against ARRAY_SIZE(chip->shadow.bytes).
> Regardless of anything, the 'bytes' array is 64 bytes large so this check disables 
> easy standard programming (TDA9874A_ESP) which has a number of 255 which is hardly the
> intended behavior.
> 
> As a matter of fact, we can think of separate check for this case like
> 	if (subaddr + 1 >= ARRAY_SIZE(chip->shadow.bytes) ||
> 	    subaddr != 0xFF) {
> 		... /* weird register, refuse */
> 	}
> but I'm not sure if there are no other special cases so for now I suggest to just disable
> it.
> 
>  drivers/media/video/tvaudio.c |   17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
> 
> Signed-off-by: Vitaly Wool <vital@embeddedalley.com> 
> 
> Index: linux-next/drivers/media/video/tvaudio.c
> ===================================================================
> --- linux-next.orig/drivers/media/video/tvaudio.c	2009-03-02 17:50:40.000000000 +0300
> +++ linux-next/drivers/media/video/tvaudio.c	2009-03-02 18:08:08.000000000 +0300
> @@ -169,13 +169,6 @@
>  			return -1;
>  		}
>  	} else {
> -		if (subaddr + 1 >= ARRAY_SIZE(chip->shadow.bytes)) {
> -			v4l2_info(sd,
> -				"Tried to access a non-existent register: %d\n",
> -				subaddr);
> -			return -EINVAL;
> -		}
> -
>  		v4l2_dbg(1, debug, sd, "chip_write: reg%d=0x%x\n",
>  			subaddr, val);
>  		chip->shadow.bytes[subaddr+1] = val;
> @@ -198,16 +191,8 @@
>  	if (mask != 0) {
>  		if (subaddr < 0) {
>  			val = (chip->shadow.bytes[1] & ~mask) | (val & mask);
> -		} else {
> -			if (subaddr + 1 >= ARRAY_SIZE(chip->shadow.bytes)) {
> -				v4l2_info(sd,
> -					"Tried to access a non-existent register: %d\n",
> -					subaddr);
> -				return -EINVAL;
> -			}
> -
> +		} else
>  			val = (chip->shadow.bytes[subaddr+1] & ~mask) | (val & mask);
> -		}
>  	}
>  	return chip_write(chip, subaddr, val);
>  }
> 
> 
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
-- 
Best regards, Klimov Alexey

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [patch] tvaudio: remove bogus check

[Mauro Carvalho Chehab]

On Mon, 02 Mar 2009 18:12:06 +0300
Vitaly Wool <vital@embeddedalley.com> wrote:

> Hello Mauro,
> 
> below is the patch that removes the subaddr check against ARRAY_SIZE(chip->shadow.bytes).
> Regardless of anything, the 'bytes' array is 64 bytes large so this check disables 
> easy standard programming (TDA9874A_ESP) which has a number of 255 which is hardly the
> intended behavior.
> 
> As a matter of fact, we can think of separate check for this case like
> 	if (subaddr + 1 >= ARRAY_SIZE(chip->shadow.bytes) ||
> 	    subaddr != 0xFF) {
> 		... /* weird register, refuse */
> 	}
> but I'm not sure if there are no other special cases so for now I suggest to just disable
> it.

This patch is wrong, since it will allow the access of an inexistent position at the shadow array:

	chip->shadow.bytes[subaddr+1] = val;

The proper fix is to increase the size of the shadow.bytes array to properly
handle the subaddr = 0xff. Something like:

-#define MAXREGS 64
+#define MAXREGS 256

Except for allocating a few more bytes, such patch won't have any other drawback.

> 
>  drivers/media/video/tvaudio.c |   17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
> 
> Signed-off-by: Vitaly Wool <vital@embeddedalley.com> 
> 
> Index: linux-next/drivers/media/video/tvaudio.c
> ===================================================================
> --- linux-next.orig/drivers/media/video/tvaudio.c	2009-03-02 17:50:40.000000000 +0300
> +++ linux-next/drivers/media/video/tvaudio.c	2009-03-02 18:08:08.000000000 +0300
> @@ -169,13 +169,6 @@
>  			return -1;
>  		}
>  	} else {
> -		if (subaddr + 1 >= ARRAY_SIZE(chip->shadow.bytes)) {
> -			v4l2_info(sd,
> -				"Tried to access a non-existent register: %d\n",
> -				subaddr);
> -			return -EINVAL;
> -		}
> -
>  		v4l2_dbg(1, debug, sd, "chip_write: reg%d=0x%x\n",
>  			subaddr, val);
>  		chip->shadow.bytes[subaddr+1] = val;
> @@ -198,16 +191,8 @@
>  	if (mask != 0) {
>  		if (subaddr < 0) {
>  			val = (chip->shadow.bytes[1] & ~mask) | (val & mask);
> -		} else {
> -			if (subaddr + 1 >= ARRAY_SIZE(chip->shadow.bytes)) {
> -				v4l2_info(sd,
> -					"Tried to access a non-existent register: %d\n",
> -					subaddr);
> -				return -EINVAL;
> -			}
> -
> +		} else
>  			val = (chip->shadow.bytes[subaddr+1] & ~mask) | (val & mask);
> -		}
>  	}
>  	return chip_write(chip, subaddr, val);
>  }
> 
> 




Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [patch] tvaudio: remove bogus check

[Vitaly Wool]

Hello Mauro,


Mauro Carvalho Chehab wrote:

> This patch is wrong, since it will allow the access of an inexistent position at the shadow array:
>
> 	chip->shadow.bytes[subaddr+1] = val;
>
> The proper fix is to increase the size of the shadow.bytes array to properly
> handle the subaddr = 0xff. Something like:
>
> -#define MAXREGS 64
> +#define MAXREGS 256
>
> Except for allocating a few more bytes, such patch won't have any other drawback.
agreed. Here's the update:

 tvaudio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Signed-off-by: Vitaly Wool <vital@embeddedalley.com>

Index: linux-next/drivers/media/video/tvaudio.c
===================================================================
--- linux-next.orig/drivers/media/video/tvaudio.c	2009-03-02 17:50:40.000000000 +0300
+++ linux-next/drivers/media/video/tvaudio.c	2009-03-03 10:35:10.000000000 +0300
@@ -54,7 +54,7 @@
 /* ---------------------------------------------------------------------- */
 /* our structs                                                            */
 
-#define MAXREGS 64
+#define MAXREGS 256
 
 struct CHIPSTATE;
 typedef int  (*getvalue)(int);


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list