All of lore.kernel.org
 help / color / mirror / Atom feed
* [media] dvb-usb: reading before start of array
@ 2013-01-09  7:36 ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-01-09  7:36 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Michael Hunold
  Cc: Jonathan Nieder, linux-media, kernel-janitors

This is a static checker fix.  In the ttusb_process_muxpack() we do:

	cc = (muxpack[len - 4] << 8) | muxpack[len - 3];

That means if we pass a number less than 4 then we will either trigger a
checksum error message or read before the start of the array.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I can't test this.

This patch doesn't introduce any bugs, but I'm not positive this is the
right thing to do.  Perhaps it's better to print an error message?

diff --git a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
index 5b682cc..99a2fd1 100644
--- a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
+++ b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
@@ -709,7 +709,7 @@ static void ttusb_process_frame(struct ttusb *ttusb, u8 * data, int len)
 			 * if length is valid and we reached the end:
 			 * goto next muxpack
 			 */
-				if ((ttusb->muxpack_ptr >= 2) &&
+				if ((ttusb->muxpack_ptr >= 4) &&
 				    (ttusb->muxpack_ptr ==
 				     ttusb->muxpack_len)) {
 					ttusb_process_muxpack(ttusb,

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [media] dvb-usb: reading before start of array
@ 2013-01-09  7:36 ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-01-09  7:36 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Michael Hunold
  Cc: Jonathan Nieder, linux-media, kernel-janitors

This is a static checker fix.  In the ttusb_process_muxpack() we do:

	cc = (muxpack[len - 4] << 8) | muxpack[len - 3];

That means if we pass a number less than 4 then we will either trigger a
checksum error message or read before the start of the array.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I can't test this.

This patch doesn't introduce any bugs, but I'm not positive this is the
right thing to do.  Perhaps it's better to print an error message?

diff --git a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
index 5b682cc..99a2fd1 100644
--- a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
+++ b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
@@ -709,7 +709,7 @@ static void ttusb_process_frame(struct ttusb *ttusb, u8 * data, int len)
 			 * if length is valid and we reached the end:
 			 * goto next muxpack
 			 */
-				if ((ttusb->muxpack_ptr >= 2) &&
+				if ((ttusb->muxpack_ptr >= 4) &&
 				    (ttusb->muxpack_ptr =
 				     ttusb->muxpack_len)) {
 					ttusb_process_muxpack(ttusb,

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [media] dvb-usb: reading before start of array
  2013-01-09  7:36 ` Dan Carpenter
@ 2013-02-05 22:10   ` Mauro Carvalho Chehab
  -1 siblings, 0 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2013-02-05 22:10 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Michael Hunold, Jonathan Nieder, linux-media, kernel-janitors

Em Wed, 9 Jan 2013 10:36:32 +0300
Dan Carpenter <dan.carpenter@oracle.com> escreveu:

> This is a static checker fix.  In the ttusb_process_muxpack() we do:
> 
> 	cc = (muxpack[len - 4] << 8) | muxpack[len - 3];
> 
> That means if we pass a number less than 4 then we will either trigger a
> checksum error message or read before the start of the array.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I can't test this.
> 
> This patch doesn't introduce any bugs, but I'm not positive this is the
> right thing to do.  Perhaps it's better to print an error message?

I don't have any ttusb device either, but i suspect that printing an
error message inside ttusb_process_muxpack() would be better.

>From what I understood, this code gets the URB data and groups it
into one TS packet (188 bytes, typically). Then, it calls 
ttusb_process_muxpack() in order to handle it.

So, the normal condition would be to always receive 188 bytes here
(usual TS packet size), except if there's something wrong with the
URB transfer.

It seems, however, that there are other issues at the logic at
ttusb_process_muxpack().

For example, from this code snippet:

        for (i = 0; i < len; i += 2)
                csum ^= le16_to_cpup((__le16 *) (muxpack + i));

an odd value for len also seems to cause troubles at this logic.

so, IMHO, the better would be to print a warning if the value is
odd or smaller than 4, and discard it.

> 
> diff --git a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
> index 5b682cc..99a2fd1 100644
> --- a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
> +++ b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
> @@ -709,7 +709,7 @@ static void ttusb_process_frame(struct ttusb *ttusb, u8 * data, int len)
>  			 * if length is valid and we reached the end:
>  			 * goto next muxpack
>  			 */
> -				if ((ttusb->muxpack_ptr >= 2) &&
> +				if ((ttusb->muxpack_ptr >= 4) &&
>  				    (ttusb->muxpack_ptr ==
>  				     ttusb->muxpack_len)) {
>  					ttusb_process_muxpack(ttusb,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 

Cheers,
Mauro

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [media] dvb-usb: reading before start of array
@ 2013-02-05 22:10   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2013-02-05 22:10 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Michael Hunold, Jonathan Nieder, linux-media, kernel-janitors

Em Wed, 9 Jan 2013 10:36:32 +0300
Dan Carpenter <dan.carpenter@oracle.com> escreveu:

> This is a static checker fix.  In the ttusb_process_muxpack() we do:
> 
> 	cc = (muxpack[len - 4] << 8) | muxpack[len - 3];
> 
> That means if we pass a number less than 4 then we will either trigger a
> checksum error message or read before the start of the array.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I can't test this.
> 
> This patch doesn't introduce any bugs, but I'm not positive this is the
> right thing to do.  Perhaps it's better to print an error message?

I don't have any ttusb device either, but i suspect that printing an
error message inside ttusb_process_muxpack() would be better.

From what I understood, this code gets the URB data and groups it
into one TS packet (188 bytes, typically). Then, it calls 
ttusb_process_muxpack() in order to handle it.

So, the normal condition would be to always receive 188 bytes here
(usual TS packet size), except if there's something wrong with the
URB transfer.

It seems, however, that there are other issues at the logic at
ttusb_process_muxpack().

For example, from this code snippet:

        for (i = 0; i < len; i += 2)
                csum ^= le16_to_cpup((__le16 *) (muxpack + i));

an odd value for len also seems to cause troubles at this logic.

so, IMHO, the better would be to print a warning if the value is
odd or smaller than 4, and discard it.

> 
> diff --git a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
> index 5b682cc..99a2fd1 100644
> --- a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
> +++ b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
> @@ -709,7 +709,7 @@ static void ttusb_process_frame(struct ttusb *ttusb, u8 * data, int len)
>  			 * if length is valid and we reached the end:
>  			 * goto next muxpack
>  			 */
> -				if ((ttusb->muxpack_ptr >= 2) &&
> +				if ((ttusb->muxpack_ptr >= 4) &&
>  				    (ttusb->muxpack_ptr =
>  				     ttusb->muxpack_len)) {
>  					ttusb_process_muxpack(ttusb,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 

Cheers,
Mauro

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [patch v2] dvb-usb: check for invalid length in ttusb_process_muxpack()
  2013-02-05 22:10   ` Mauro Carvalho Chehab
@ 2013-02-07  8:24     ` Dan Carpenter
  -1 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-02-07  8:24 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: linux-media, kernel-janitors

This patch is driven by a static checker warning.

The ttusb_process_muxpack() function is only called from
ttusb_process_frame().  Before calling, it verifies that len >= 2.  The
problem is that len == 2 is not valid and would lead to an array
underflow.

Odd number values for len are also invalid and would lead to reading
past the end of the array.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Moved the check from the caller into the function.  Added a check
for odd values.  Added an error message.  Increment the numinvalid
counter.

diff --git a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
index 5b682cc..e407185 100644
--- a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
+++ b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
@@ -561,6 +561,13 @@ static void ttusb_process_muxpack(struct ttusb *ttusb, const u8 * muxpack,
 {
 	u16 csum = 0, cc;
 	int i;
+
+	if (len < 4 || len & 0x1) {
+		pr_warn("%s: muxpack has invalid len %d\n", __func__, len);
+		numinvalid++;
+		return;
+	}
+
 	for (i = 0; i < len; i += 2)
 		csum ^= le16_to_cpup((__le16 *) (muxpack + i));
 	if (csum) {

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [patch v2] dvb-usb: check for invalid length in ttusb_process_muxpack()
@ 2013-02-07  8:24     ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-02-07  8:24 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: linux-media, kernel-janitors

This patch is driven by a static checker warning.

The ttusb_process_muxpack() function is only called from
ttusb_process_frame().  Before calling, it verifies that len >= 2.  The
problem is that len = 2 is not valid and would lead to an array
underflow.

Odd number values for len are also invalid and would lead to reading
past the end of the array.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Moved the check from the caller into the function.  Added a check
for odd values.  Added an error message.  Increment the numinvalid
counter.

diff --git a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
index 5b682cc..e407185 100644
--- a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
+++ b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
@@ -561,6 +561,13 @@ static void ttusb_process_muxpack(struct ttusb *ttusb, const u8 * muxpack,
 {
 	u16 csum = 0, cc;
 	int i;
+
+	if (len < 4 || len & 0x1) {
+		pr_warn("%s: muxpack has invalid len %d\n", __func__, len);
+		numinvalid++;
+		return;
+	}
+
 	for (i = 0; i < len; i += 2)
 		csum ^= le16_to_cpup((__le16 *) (muxpack + i));
 	if (csum) {

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-07  8:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-09  7:36 [media] dvb-usb: reading before start of array Dan Carpenter
2013-01-09  7:36 ` Dan Carpenter
2013-02-05 22:10 ` Mauro Carvalho Chehab
2013-02-05 22:10   ` Mauro Carvalho Chehab
2013-02-07  8:24   ` [patch v2] dvb-usb: check for invalid length in ttusb_process_muxpack() Dan Carpenter
2013-02-07  8:24     ` Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.