* [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.