* Re: [PATCH][RESEND] Fix a potential NULL pointer deref in the aic7xxx, ahc_print_register() function [not found] <200708042030.52405.jesper.juhl@gmail.com> @ 2007-08-04 19:43 ` James Bottomley 2007-08-05 15:36 ` Jesper Juhl 0 siblings, 1 reply; 5+ messages in thread From: James Bottomley @ 2007-08-04 19:43 UTC (permalink / raw) To: Jesper Juhl Cc: Andrew Morton, James Bottomley, linux-scsi, Justin T. Gibbs, Linux Kernel Mailing List On Sat, 2007-08-04 at 20:30 +0200, Jesper Juhl wrote: > (resend of patch previously submitted on 28-Jul-2007 23:06) > > > Ehlo, > > The Coverity checker noticed that we have a potential NULL pointer > deref in drivers/scsi/aic7xxx/aic7xxx_core.c::ahc_print_register(). > This patch handles it by adding the same test against NULL that is > used elsewhere in the same function. It's on my list of things to look at ... but not very high. I suspect it actually isn't triggerable, but if you can tell me how, it will save me from looking. James ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][RESEND] Fix a potential NULL pointer deref in the aic7xxx, ahc_print_register() function 2007-08-04 19:43 ` [PATCH][RESEND] Fix a potential NULL pointer deref in the aic7xxx, ahc_print_register() function James Bottomley @ 2007-08-05 15:36 ` Jesper Juhl 2007-08-05 15:42 ` Justin T. Gibbs 2007-08-05 15:53 ` James Bottomley 0 siblings, 2 replies; 5+ messages in thread From: Jesper Juhl @ 2007-08-05 15:36 UTC (permalink / raw) To: James Bottomley Cc: Andrew Morton, James Bottomley, linux-scsi, Justin T. Gibbs, Linux Kernel Mailing List On 04/08/07, James Bottomley <James.Bottomley@steeleye.com> wrote: > On Sat, 2007-08-04 at 20:30 +0200, Jesper Juhl wrote: > > (resend of patch previously submitted on 28-Jul-2007 23:06) > > > > > > Ehlo, > > > > The Coverity checker noticed that we have a potential NULL pointer > > deref in drivers/scsi/aic7xxx/aic7xxx_core.c::ahc_print_register(). > > This patch handles it by adding the same test against NULL that is > > used elsewhere in the same function. > > It's on my list of things to look at ... but not very high. I suspect > it actually isn't triggerable, but if you can tell me how, it will save > me from looking. > Here's what Coverity reported : ... 6525 int 6526 ahc_print_register(ahc_reg_parse_entry_t *table, u_int num_entries, 6527 const char *name, u_int address, u_int value, 6528 u_int *cur_column, u_int wrap_point) 6529 { 6530 int printed; 6531 u_int printed_mask; 6532 Event var_compare_op: Added "cur_column" due to comparison "cur_column != 0" Also see events: [var_deref_op] At conditional (1): "cur_column != 0" taking false path 6533 if (cur_column != NULL && *cur_column >= wrap_point) { 6534 printf("\n"); 6535 *cur_column = 0; 6536 } 6537 printed = printf("%s[0x%x]", name, value); At conditional (2): "table == 0" taking true path 6538 if (table == NULL) { 6539 printed += printf(" "); Event var_deref_op: Variable "cur_column" tracked as NULL was dereferenced. Also see events: [var_compare_op] 6540 *cur_column += printed; 6541 return (printed); 6542 } ... So it requires a NULL 'table' and a != NULL 'cur_column' to trigger. Whether or not that's actually possible I'm not sure, but it seems safer to guard against it :) By the way; if this can actually be triggered, then ahd_print_register() has the same problem. -- Jesper Juhl <jesper.juhl@gmail.com> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][RESEND] Fix a potential NULL pointer deref in the aic7xxx, ahc_print_register() function 2007-08-05 15:36 ` Jesper Juhl @ 2007-08-05 15:42 ` Justin T. Gibbs 2007-08-05 15:52 ` Jesper Juhl 2007-08-05 15:53 ` James Bottomley 1 sibling, 1 reply; 5+ messages in thread From: Justin T. Gibbs @ 2007-08-05 15:42 UTC (permalink / raw) To: Jesper Juhl Cc: James Bottomley, Andrew Morton, James Bottomley, linux-scsi, Linux Kernel Mailing List All of this logic was simplified back in '05 in the BSD drivers by adding this to the top of the function: u_int dummy_column; if (cur_column == NULL) { dummy_column = 0; cur_column = &dummy_column; } and then stripping out the cur_column == NULL checks in the routine. -- Justin Jesper Juhl wrote: > On 04/08/07, James Bottomley <James.Bottomley@steeleye.com> wrote: >> On Sat, 2007-08-04 at 20:30 +0200, Jesper Juhl wrote: >>> (resend of patch previously submitted on 28-Jul-2007 23:06) >>> >>> >>> Ehlo, >>> >>> The Coverity checker noticed that we have a potential NULL pointer >>> deref in drivers/scsi/aic7xxx/aic7xxx_core.c::ahc_print_register(). >>> This patch handles it by adding the same test against NULL that is >>> used elsewhere in the same function. >> It's on my list of things to look at ... but not very high. I suspect >> it actually isn't triggerable, but if you can tell me how, it will save >> me from looking. >> > > Here's what Coverity reported : > ... > 6525 int > 6526 ahc_print_register(ahc_reg_parse_entry_t *table, u_int num_entries, > 6527 const char *name, u_int address, u_int value, > 6528 u_int *cur_column, u_int wrap_point) > 6529 { > 6530 int printed; > 6531 u_int printed_mask; > 6532 > > Event var_compare_op: Added "cur_column" due to comparison "cur_column != 0" > Also see events: [var_deref_op] > At conditional (1): "cur_column != 0" taking false path > > 6533 if (cur_column != NULL && *cur_column >= wrap_point) { > 6534 printf("\n"); > 6535 *cur_column = 0; > 6536 } > 6537 printed = printf("%s[0x%x]", name, value); > > At conditional (2): "table == 0" taking true path > > 6538 if (table == NULL) { > 6539 printed += printf(" "); > > Event var_deref_op: Variable "cur_column" tracked as NULL was dereferenced. > Also see events: [var_compare_op] > > 6540 *cur_column += printed; > 6541 return (printed); > 6542 } > ... > > So it requires a NULL 'table' and a != NULL 'cur_column' to trigger. > Whether or not that's actually possible I'm not sure, but it seems > safer to guard against it :) > > > By the way; if this can actually be triggered, then > ahd_print_register() has the same problem. > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][RESEND] Fix a potential NULL pointer deref in the aic7xxx, ahc_print_register() function 2007-08-05 15:42 ` Justin T. Gibbs @ 2007-08-05 15:52 ` Jesper Juhl 0 siblings, 0 replies; 5+ messages in thread From: Jesper Juhl @ 2007-08-05 15:52 UTC (permalink / raw) To: Justin T. Gibbs Cc: James Bottomley, Andrew Morton, James Bottomley, linux-scsi, Linux Kernel Mailing List, Jesper Juhl On Sunday 05 August 2007 17:42:31 Justin T. Gibbs wrote: > All of this logic was simplified back in '05 in the BSD drivers by adding > this to the top of the function: > > u_int dummy_column; > > if (cur_column == NULL) { > dummy_column = 0; > cur_column = &dummy_column; > } > > and then stripping out the cur_column == NULL checks in the routine. > Thank you for that info. James, if that sounds like a better way to deal with it to you, then here's a patch to implement it. Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com> --- drivers/scsi/aic7xxx/aic7xxx_core.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/scsi/aic7xxx/aic7xxx_core.c b/drivers/scsi/aic7xxx/aic7xxx_core.c index 75733b0..d1f3f25 100644 --- a/drivers/scsi/aic7xxx/aic7xxx_core.c +++ b/drivers/scsi/aic7xxx/aic7xxx_core.c @@ -6529,8 +6529,14 @@ ahc_print_register(ahc_reg_parse_entry_t *table, u_int num_entries, { int printed; u_int printed_mask; + u_int dummy_column; - if (cur_column != NULL && *cur_column >= wrap_point) { + if (!cur_column) { + dummy_column = 0; + cur_column = &dummy_column; + } + + if (*cur_column >= wrap_point) { printf("\n"); *cur_column = 0; } @@ -6565,8 +6571,7 @@ ahc_print_register(ahc_reg_parse_entry_t *table, u_int num_entries, printed += printf(") "); else printed += printf(" "); - if (cur_column != NULL) - *cur_column += printed; + *cur_column += printed; return (printed); } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH][RESEND] Fix a potential NULL pointer deref in the aic7xxx, ahc_print_register() function 2007-08-05 15:36 ` Jesper Juhl 2007-08-05 15:42 ` Justin T. Gibbs @ 2007-08-05 15:53 ` James Bottomley 1 sibling, 0 replies; 5+ messages in thread From: James Bottomley @ 2007-08-05 15:53 UTC (permalink / raw) To: Jesper Juhl Cc: Andrew Morton, linux-scsi, Justin T. Gibbs, Linux Kernel Mailing List On Sun, 2007-08-05 at 17:36 +0200, Jesper Juhl wrote: > On 04/08/07, James Bottomley <James.Bottomley@steeleye.com> wrote: > > On Sat, 2007-08-04 at 20:30 +0200, Jesper Juhl wrote: > > > (resend of patch previously submitted on 28-Jul-2007 23:06) > > > > > > > > > Ehlo, > > > > > > The Coverity checker noticed that we have a potential NULL pointer > > > deref in drivers/scsi/aic7xxx/aic7xxx_core.c::ahc_print_register(). > > > This patch handles it by adding the same test against NULL that is > > > used elsewhere in the same function. > > > > It's on my list of things to look at ... but not very high. I suspect > > it actually isn't triggerable, but if you can tell me how, it will save > > me from looking. > > > > Here's what Coverity reported : > ... > 6525 int > 6526 ahc_print_register(ahc_reg_parse_entry_t *table, u_int num_entries, > 6527 const char *name, u_int address, u_int value, > 6528 u_int *cur_column, u_int wrap_point) > 6529 { > 6530 int printed; > 6531 u_int printed_mask; > 6532 > > Event var_compare_op: Added "cur_column" due to comparison "cur_column != 0" > Also see events: [var_deref_op] > At conditional (1): "cur_column != 0" taking false path > > 6533 if (cur_column != NULL && *cur_column >= wrap_point) { > 6534 printf("\n"); > 6535 *cur_column = 0; > 6536 } > 6537 printed = printf("%s[0x%x]", name, value); > > At conditional (2): "table == 0" taking true path > > 6538 if (table == NULL) { > 6539 printed += printf(" "); > > Event var_deref_op: Variable "cur_column" tracked as NULL was dereferenced. > Also see events: [var_compare_op] > > 6540 *cur_column += printed; > 6541 return (printed); > 6542 } > ... > > So it requires a NULL 'table' and a != NULL 'cur_column' to trigger. > Whether or not that's actually possible I'm not sure, but it seems > safer to guard against it :) > > > By the way; if this can actually be triggered, then > ahd_print_register() has the same problem. But this is precisely the point. A large number of drivers have purely internal functions, which this is. Most often, because the API is purely internal to the driver, misuse is reported via BUG_ON (supposed to be picked up by the driver writer in their testing). The problem with this is that we never do: BUG_ON(ptr==NULL); Because a simple use of the null pointer will also trigger the bug, so we don't need the check. I suspect table == NULL requires cur_column != NULL as part of the internal API and there's no BUG_ON checking for it because the use of the pointer would be the BUG. If you blindly go checking for NULL and coping with it, you've actually weakened our internal API checking, which definitely isn't the object of the exercise. Worse, becase we have no maintainer for this driver someone touching it could easily get this wrong and a subtle and much harder to find bug will be introduced. So, as I said, if you can prove table == NULL && cur_column == NULL should be a genuine use case of the API then by all means, this can go in. James ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-08-05 16:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200708042030.52405.jesper.juhl@gmail.com>
2007-08-04 19:43 ` [PATCH][RESEND] Fix a potential NULL pointer deref in the aic7xxx, ahc_print_register() function James Bottomley
2007-08-05 15:36 ` Jesper Juhl
2007-08-05 15:42 ` Justin T. Gibbs
2007-08-05 15:52 ` Jesper Juhl
2007-08-05 15:53 ` James Bottomley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox