| Message ID | 4A2F50E0.8030404@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hei Jan, On 06/10/2009 09:21 AM, Jan Nikitenko wrote: > This patch fixes stack corruption bug present in dump_regs function of > zl10353 and qt1010 drivers: > the buffer buf is one byte smaller than required - there is 4 chars > for address prefix, 16*3 chars for dump of 16 eeprom bytes per line > and 1 byte for zero ending the string required, i.e. 53 bytes, but > only 52 were provided. > The one byte missing in stack based buffer buf can cause stack > corruption possibly leading to kernel oops, as discovered originally > with af9015 driver. > > Signed-off-by: Jan Nikitenko <jan.nikitenko@gmail.com> > > --- > > Antti Palosaari wrote: > > On 06/10/2009 01:39 AM, Jan Nikitenko wrote: > >> Solved with "[PATCH] af9015: fix stack corruption bug". > > > > This error leads to the zl10353.c and there it was copied to qt1010.c > > and af9015.c. > > > Antti, thanks for pointing out that the same problem was also in > zl10353.c and qt1010.c. Include your Sign-off-by, please. I tried to test that patch (from patchwork) to ensure it is OK before ack, but I found it does not apply for reason or other. It looks correct for my eyes. Please check what's wrong and apply new patch. [crope@localhost v4l-dvb]$ patch -p1 < af9015-fix-stack-corruption-bug.patch patching file linux/drivers/media/dvb/dvb-usb/af9015.c [crope@localhost v4l-dvb]$ patch -p1 < zl10353-and-qt1010-fix-stack-corruption-bug.patch patching file linux/drivers/media/common/tuners/qt1010.c Hunk #1 FAILED at 65. 1 out of 1 hunk FAILED -- saving rejects to file linux/drivers/media/common/tuners/qt1010.c.rej patching file linux/drivers/media/dvb/frontends/zl10353.c Hunk #1 FAILED at 102. 1 out of 1 hunk FAILED -- saving rejects to file linux/drivers/media/dvb/frontends/zl10353.c.rej [crope@localhost v4l-dvb]$ hg diff diff -r 148b4c93a728 linux/drivers/media/dvb/dvb-usb/af9015.c --- a/linux/drivers/media/dvb/dvb-usb/af9015.c Mon Jun 15 14:15:33 2009 -0300 +++ b/linux/drivers/media/dvb/dvb-usb/af9015.c Mon Jun 15 21:55:55 2009 +0300 @@ -541,7 +541,7 @@ /* dump eeprom */ static int af9015_eeprom_dump(struct dvb_usb_device *d) { - char buf[52], buf2[4]; + char buf[4+3*16+1], buf2[4]; u8 reg, val; for (reg = 0; ; reg++) { [crope@localhost v4l-dvb]$ hg head changeset: 11978:148b4c93a728 tag: tip parent: 11975:144d8d0cebc5 parent: 11977:8b416ba3ac89 user: Mauro Carvalho Chehab <mchehab@redhat.com> date: Mon Jun 15 14:15:33 2009 -0300 summary: merge: http://www.linuxtv.org/hg/~dougsland/em28xx regards Antti
Em Wed, 10 Jun 2009 08:21:20 +0200 Jan Nikitenko <jan.nikitenko@gmail.com> escreveu: > This patch fixes stack corruption bug present in dump_regs function of zl10353 > and qt1010 drivers: > the buffer buf is one byte smaller than required - there is 4 chars > for address prefix, 16*3 chars for dump of 16 eeprom bytes per line > and 1 byte for zero ending the string required, i.e. 53 bytes, but > only 52 were provided. > The one byte missing in stack based buffer buf can cause stack corruption > possibly leading to kernel oops, as discovered originally with af9015 driver. > > Signed-off-by: Jan Nikitenko <jan.nikitenko@gmail.com> > > --- > > Antti Palosaari wrote: > > On 06/10/2009 01:39 AM, Jan Nikitenko wrote: > >> Solved with "[PATCH] af9015: fix stack corruption bug". > > > > This error leads to the zl10353.c and there it was copied to qt1010.c > > and af9015.c. > > > Antti, thanks for pointing out that the same problem was also in zl10353.c and > qt1010.c. Include your Sign-off-by, please. > > Best regards, > Jan > > linux/drivers/media/common/tuners/qt1010.c | 2 +- > linux/drivers/media/dvb/frontends/zl10353.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff -r cff06234b725 linux/drivers/media/common/tuners/qt1010.c > --- a/linux/drivers/media/common/tuners/qt1010.c Sun May 31 23:07:01 2009 +0300 > +++ b/linux/drivers/media/common/tuners/qt1010.c Wed Jun 10 07:37:51 2009 +0200 > @@ -65,7 +65,7 @@ > /* dump all registers */ > static void qt1010_dump_regs(struct qt1010_priv *priv) > { > - char buf[52], buf2[4]; > + char buf[4+3*16+1], buf2[4]; CodingStyle is incorrect. It should be buf[4 + 3 * 16 + 1]. > u8 reg, val; > > for (reg = 0; ; reg++) { > diff -r cff06234b725 linux/drivers/media/dvb/frontends/zl10353.c > --- a/linux/drivers/media/dvb/frontends/zl10353.c Sun May 31 23:07:01 2009 +0300 > +++ b/linux/drivers/media/dvb/frontends/zl10353.c Wed Jun 10 07:37:51 2009 +0200 > @@ -102,7 +102,7 @@ > static void zl10353_dump_regs(struct dvb_frontend *fe) > { > struct zl10353_state *state = fe->demodulator_priv; > - char buf[52], buf2[4]; > + char buf[4+3*16+1], buf2[4]; Same CodingStyle issue here. > int ret; > u8 reg; > Without having actually looking at the source code, would it be possible to change the logic in order to use something else instead of a magic number for buf size - e. g. using sizeof(something) ? > > -- > 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 -- 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
Mauro Carvalho Chehab wrote: > Em Wed, 10 Jun 2009 08:21:20 +0200 > Jan Nikitenko <jan.nikitenko@gmail.com> escreveu: > >> This patch fixes stack corruption bug present in dump_regs function of zl10353 >> and qt1010 drivers: >> the buffer buf is one byte smaller than required - there is 4 chars >> for address prefix, 16*3 chars for dump of 16 eeprom bytes per line >> and 1 byte for zero ending the string required, i.e. 53 bytes, but >> only 52 were provided. >> The one byte missing in stack based buffer buf can cause stack corruption >> possibly leading to kernel oops, as discovered originally with af9015 driver. >> >> Signed-off-by: Jan Nikitenko <jan.nikitenko@gmail.com> >> >> --- >> >> Antti Palosaari wrote: >> > On 06/10/2009 01:39 AM, Jan Nikitenko wrote: >> >> Solved with "[PATCH] af9015: fix stack corruption bug". >> > >> > This error leads to the zl10353.c and there it was copied to qt1010.c >> > and af9015.c. >> > >> Antti, thanks for pointing out that the same problem was also in zl10353.c and >> qt1010.c. Include your Sign-off-by, please. >> >> Best regards, >> Jan >> >> linux/drivers/media/common/tuners/qt1010.c | 2 +- >> linux/drivers/media/dvb/frontends/zl10353.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff -r cff06234b725 linux/drivers/media/common/tuners/qt1010.c >> --- a/linux/drivers/media/common/tuners/qt1010.c Sun May 31 23:07:01 2009 +0300 >> +++ b/linux/drivers/media/common/tuners/qt1010.c Wed Jun 10 07:37:51 2009 +0200 >> @@ -65,7 +65,7 @@ >> /* dump all registers */ >> static void qt1010_dump_regs(struct qt1010_priv *priv) >> { >> - char buf[52], buf2[4]; >> + char buf[4+3*16+1], buf2[4]; > > CodingStyle is incorrect. It should be buf[4 + 3 * 16 + 1]. right. > > >> u8 reg, val; >> >> for (reg = 0; ; reg++) { >> diff -r cff06234b725 linux/drivers/media/dvb/frontends/zl10353.c >> --- a/linux/drivers/media/dvb/frontends/zl10353.c Sun May 31 23:07:01 2009 +0300 >> +++ b/linux/drivers/media/dvb/frontends/zl10353.c Wed Jun 10 07:37:51 2009 +0200 >> @@ -102,7 +102,7 @@ >> static void zl10353_dump_regs(struct dvb_frontend *fe) >> { >> struct zl10353_state *state = fe->demodulator_priv; >> - char buf[52], buf2[4]; >> + char buf[4+3*16+1], buf2[4]; > > Same CodingStyle issue here. agreed. > >> int ret; >> u8 reg; >> > > Without having actually looking at the source code, would it be possible to > change the logic in order to use something else instead of a magic number for > buf size - e. g. using sizeof(something) ? I am not sure if that's possible - the buffer is used for sprintf in a loop to store text representation of registers dump, before printk-ing it. We could put there a comment, suggesting that 4 bytes are required for address prefix of form "00: ", then 16 strings of form "00 " (3 bytes each) and one byte for zero to terminate the string. Or we could use sizeof, like this: char buf[sizeof("00: ") - 1 + 16 * (sizeof("00 ") - 1) + 1] or char buf[sizeof("00: 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f ")] but it is not very readable in my opinion either. Maybe the best way would be to avoid the need for temporal buffer completely by directly using printk in a loop, that is only the first printk with KERN_DEBUG, followed by sequence of printk with registers dump and final printk with end of line (but isn't a printk without KERN_ facility coding style problem as well?). I am not sure, what approach is the best - I just wanted to fix a bug, which did not allow to use my af9015 based tuner on mips platform. After that, Antti pointed out, that the same code with the same bug is also in other two sources, so I send the same fix for them as well... Best regards, Jan -- 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
On Mittwoch, 17. Juni 2009, Jan Nikitenko wrote: > > Or we could use sizeof, like this: > char buf[sizeof("00: ") - 1 + 16 * (sizeof("00 ") - 1) + 1] > or > char buf[sizeof("00: 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f > ")] but it is not very readable in my opinion either. > > Maybe the best way would be to avoid the need for temporal buffer > completely by directly using printk in a loop, that is only the first > printk with KERN_DEBUG, followed by sequence of printk with registers dump > and final printk with end of line (but isn't a printk without KERN_ > facility coding style problem as well?). > Exactly for this case, line continuation, there is KERN_CONT defined. Regards Matthias -- 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
Em Wed, 17 Jun 2009 14:26:28 +0200 Matthias Schwarzott <zzam@gentoo.org> escreveu: > On Mittwoch, 17. Juni 2009, Jan Nikitenko wrote: > > > > Or we could use sizeof, like this: > > char buf[sizeof("00: ") - 1 + 16 * (sizeof("00 ") - 1) + 1] > > or > > char buf[sizeof("00: 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f > > ")] but it is not very readable in my opinion either. > > > > Maybe the best way would be to avoid the need for temporal buffer > > completely by directly using printk in a loop, that is only the first > > printk with KERN_DEBUG, followed by sequence of printk with registers dump > > and final printk with end of line (but isn't a printk without KERN_ > > facility coding style problem as well?). > > > > Exactly for this case, line continuation, there is KERN_CONT defined. There are some functions meant for printing hex dumps at kernel.h: extern void hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize, char *linebuf, size_t linebuflen, bool ascii); extern void print_hex_dump(const char *level, const char *prefix_str, int prefix_type, int rowsize, int groupsize, const void *buf, size_t len, bool ascii); extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type, const void *buf, size_t len); Also, it is possible to use kasprintf() to dynamically allocate a temporary buffer. If you use it, you'll need to do a kfree after its usage. Cheers, Mauro -- 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
diff -r cff06234b725 linux/drivers/media/common/tuners/qt1010.c --- a/linux/drivers/media/common/tuners/qt1010.c Sun May 31 23:07:01 2009 +0300 +++ b/linux/drivers/media/common/tuners/qt1010.c Wed Jun 10 07:37:51 2009 +0200 @@ -65,7 +65,7 @@ /* dump all registers */ static void qt1010_dump_regs(struct qt1010_priv *priv) { - char buf[52], buf2[4]; + char buf[4+3*16+1], buf2[4]; u8 reg, val; for (reg = 0; ; reg++) { diff -r cff06234b725 linux/drivers/media/dvb/frontends/zl10353.c --- a/linux/drivers/media/dvb/frontends/zl10353.c Sun May 31 23:07:01 2009 +0300 +++ b/linux/drivers/media/dvb/frontends/zl10353.c Wed Jun 10 07:37:51 2009 +0200 @@ -102,7 +102,7 @@ static void zl10353_dump_regs(struct dvb_frontend *fe) { struct zl10353_state *state = fe->demodulator_priv; - char buf[52], buf2[4]; + char buf[4+3*16+1], buf2[4]; int ret; u8 reg;