* [patch] isdn/gigaset: off by one check leading to oops @ 2013-01-17 7:44 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2013-01-17 7:44 UTC (permalink / raw) To: Hansjoerg Lipp Cc: Tilman Schmidt, Karsten Keil, gigaset307x-common, netdev, kernel-janitors If l = 12 then later we subtract 12 leaving zero. We do a zero size allocation, so "dbgline" points to the ZERO_SIZE_PTR. It leads to an oops when we set the NUL terminator: dbgline[3 * l - 1] = '\0'; Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- Static analysis stuff. diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c index 68452b7..0d34325 100644 --- a/drivers/isdn/gigaset/capi.c +++ b/drivers/isdn/gigaset/capi.c @@ -239,7 +239,7 @@ static inline void dump_rawmsg(enum debuglevel level, const char *tag, return; l = CAPIMSG_LEN(data); - if (l < 12) { + if (l <= 12) { gig_dbg(level, "%s: ??? LEN=%04d", tag, l); return; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [patch] isdn/gigaset: off by one check leading to oops @ 2013-01-17 7:44 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2013-01-17 7:44 UTC (permalink / raw) To: Hansjoerg Lipp Cc: Tilman Schmidt, Karsten Keil, gigaset307x-common, netdev, kernel-janitors If l == 12 then later we subtract 12 leaving zero. We do a zero size allocation, so "dbgline" points to the ZERO_SIZE_PTR. It leads to an oops when we set the NUL terminator: dbgline[3 * l - 1] = '\0'; Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- Static analysis stuff. diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c index 68452b7..0d34325 100644 --- a/drivers/isdn/gigaset/capi.c +++ b/drivers/isdn/gigaset/capi.c @@ -239,7 +239,7 @@ static inline void dump_rawmsg(enum debuglevel level, const char *tag, return; l = CAPIMSG_LEN(data); - if (l < 12) { + if (l <= 12) { gig_dbg(level, "%s: ??? LEN=%04d", tag, l); return; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch] isdn/gigaset: off by one check leading to oops 2013-01-17 7:44 ` Dan Carpenter @ 2013-01-18 12:18 ` Tilman Schmidt -1 siblings, 0 replies; 6+ messages in thread From: Tilman Schmidt @ 2013-01-18 12:18 UTC (permalink / raw) To: Dan Carpenter Cc: Hansjoerg Lipp, Karsten Keil, gigaset307x-common, netdev, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 1487 bytes --] Hi Dan, Am 17.01.2013 08:44, schrieb Dan Carpenter: > If l == 12 then later we subtract 12 leaving zero. We do a zero size > allocation, so "dbgline" points to the ZERO_SIZE_PTR. It leads to an > oops when we set the NUL terminator: > dbgline[3 * l - 1] = '\0'; thanks for finding that bug, but NAK to your fix. > @@ -239,7 +239,7 @@ static inline void dump_rawmsg(enum debuglevel level, const char *tag, > return; > > l = CAPIMSG_LEN(data); > - if (l < 12) { > + if (l <= 12) { > gig_dbg(level, "%s: ??? LEN=%04d", tag, l); > return; > } CAPI messages of exactly 12 bytes are legal, and should be decoded with the regular gig_dbg() call immediately after that hunk. It's just the hex dump part that should be skipped in that case. So I'd prefer to have it fixed this way instead: @@ -248,6 +248,8 @@ static inline void dump_rawmsg(enum debuglevel level, const char *tag, CAPIMSG_APPID(data), CAPIMSG_MSGID(data), l, CAPIMSG_CONTROL(data)); l -= 12; + if (l <= 0) + return; dbgline = kmalloc(3 * l, GFP_ATOMIC); if (!dbgline) return; I'll prepare a patch of my own, citing you as reporter, if that's ok with you. Thanks, Tilman -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 260 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] isdn/gigaset: off by one check leading to oops @ 2013-01-18 12:18 ` Tilman Schmidt 0 siblings, 0 replies; 6+ messages in thread From: Tilman Schmidt @ 2013-01-18 12:18 UTC (permalink / raw) To: Dan Carpenter Cc: Hansjoerg Lipp, Karsten Keil, gigaset307x-common, netdev, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 1487 bytes --] Hi Dan, Am 17.01.2013 08:44, schrieb Dan Carpenter: > If l == 12 then later we subtract 12 leaving zero. We do a zero size > allocation, so "dbgline" points to the ZERO_SIZE_PTR. It leads to an > oops when we set the NUL terminator: > dbgline[3 * l - 1] = '\0'; thanks for finding that bug, but NAK to your fix. > @@ -239,7 +239,7 @@ static inline void dump_rawmsg(enum debuglevel level, const char *tag, > return; > > l = CAPIMSG_LEN(data); > - if (l < 12) { > + if (l <= 12) { > gig_dbg(level, "%s: ??? LEN=%04d", tag, l); > return; > } CAPI messages of exactly 12 bytes are legal, and should be decoded with the regular gig_dbg() call immediately after that hunk. It's just the hex dump part that should be skipped in that case. So I'd prefer to have it fixed this way instead: @@ -248,6 +248,8 @@ static inline void dump_rawmsg(enum debuglevel level, const char *tag, CAPIMSG_APPID(data), CAPIMSG_MSGID(data), l, CAPIMSG_CONTROL(data)); l -= 12; + if (l <= 0) + return; dbgline = kmalloc(3 * l, GFP_ATOMIC); if (!dbgline) return; I'll prepare a patch of my own, citing you as reporter, if that's ok with you. Thanks, Tilman -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 260 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] isdn/gigaset: off by one check leading to oops 2013-01-18 12:18 ` Tilman Schmidt @ 2013-01-18 12:26 ` Dan Carpenter -1 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2013-01-18 12:26 UTC (permalink / raw) To: Tilman Schmidt Cc: Hansjoerg Lipp, Karsten Keil, gigaset307x-common, netdev, kernel-janitors On Fri, Jan 18, 2013 at 01:18:00PM +0100, Tilman Schmidt wrote: > I'll prepare a patch of my own, citing you as reporter, if that's > ok with you. > That sounds great. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] isdn/gigaset: off by one check leading to oops @ 2013-01-18 12:26 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2013-01-18 12:26 UTC (permalink / raw) To: Tilman Schmidt Cc: Hansjoerg Lipp, Karsten Keil, gigaset307x-common, netdev, kernel-janitors On Fri, Jan 18, 2013 at 01:18:00PM +0100, Tilman Schmidt wrote: > I'll prepare a patch of my own, citing you as reporter, if that's > ok with you. > That sounds great. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-18 12:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-17 7:44 [patch] isdn/gigaset: off by one check leading to oops Dan Carpenter 2013-01-17 7:44 ` Dan Carpenter 2013-01-18 12:18 ` Tilman Schmidt 2013-01-18 12:18 ` Tilman Schmidt 2013-01-18 12:26 ` Dan Carpenter 2013-01-18 12:26 ` 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.