On Wed, 16 Apr 2008, Cyrill Gorcunov wrote:
> Fix potential NULL pointer dereference if kstrdup failed
>
> Signed-off-by: Cyrill Gorcunov

Acked-by: Henrique de Moraes Holschuh

> Index: linux-2.6.git/drivers/misc/thinkpad_acpi.c
> ================================================== =================
> --- linux-2.6.git.orig/drivers/misc/thinkpad_acpi.c 2008-04-16 20:35:34.000000000 +0400
> +++ linux-2.6.git/drivers/misc/thinkpad_acpi.c 2008-04-16 20:36:38.000000000 +0400
> @@ -5826,7 +5826,7 @@ static void __init get_thinkpad_model_da
>
> tp->model_str = kstrdup(dmi_get_system_info(DMI_PRODUCT_VERSION),
> GFP_KERNEL);
> - if (strnicmp(tp->model_str, "ThinkPad", 8) != 0) {
> + if (tp->model_str && strnicmp(tp->model_str, "ThinkPad", 8) != 0) {
> kfree(tp->model_str);
> tp->model_str = NULL;
> }
>
> --

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi!

> Fix potential NULL pointer dereference if kstrdup failed
>
> Signed-off-by: Cyrill Gorcunov
>
> ---
>
> Index: linux-2.6.git/drivers/misc/thinkpad_acpi.c
> ================================================== =================
> --- linux-2.6.git.orig/drivers/misc/thinkpad_acpi.c 2008-04-16 20:35:34.000000000 +0400
> +++ linux-2.6.git/drivers/misc/thinkpad_acpi.c 2008-04-16 20:36:38.000000000 +0400
> @@ -5826,7 +5826,7 @@ static void __init get_thinkpad_model_da
>
> tp->model_str = kstrdup(dmi_get_system_info(DMI_PRODUCT_VERSION),
> GFP_KERNEL);
> - if (strnicmp(tp->model_str, "ThinkPad", 8) != 0) {
> + if (tp->model_str && strnicmp(tp->model_str, "ThinkPad", 8) != 0) {
> kfree(tp->model_str);
> tp->model_str = NULL;
> }

are you sure? This seems to assume machine is thinkpad if kstrdup
fails... which is very wrong.

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pav...rses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Pavel Machek - Fri, Apr 18, 2008 at 02:41:12PM +0200]
| Hi!
|
| > Fix potential NULL pointer dereference if kstrdup failed
| >
| > Signed-off-by: Cyrill Gorcunov
| >
| > ---
| >
| > Index: linux-2.6.git/drivers/misc/thinkpad_acpi.c
| > ================================================== =================
| > --- linux-2.6.git.orig/drivers/misc/thinkpad_acpi.c 2008-04-16 20:35:34.000000000 +0400
| > +++ linux-2.6.git/drivers/misc/thinkpad_acpi.c 2008-04-16 20:36:38.000000000 +0400
| > @@ -5826,7 +5826,7 @@ static void __init get_thinkpad_model_da
| >
| > tp->model_str = kstrdup(dmi_get_system_info(DMI_PRODUCT_VERSION),
| > GFP_KERNEL);
| > - if (strnicmp(tp->model_str, "ThinkPad", 8) != 0) {
| > + if (tp->model_str && strnicmp(tp->model_str, "ThinkPad", 8) != 0) {
| > kfree(tp->model_str);
| > tp->model_str = NULL;
| > }
|
| are you sure? This seems to assume machine is thinkpad if kstrdup
| fails... which is very wrong.

No, it's *not* wrong, look there we have

tp->model_str = kstrdup(dmi_get_system_info(DMI_PRODUCT_VERSION),
GFP_KERNEL);

lets assume we've got NULL here so probe_for_thinkpad() will check for it

is_thinkpad = (thinkpad_id.model_str != NULL);


Thanks for comment

- Cyrill -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Pavel Machek - Fri, Apr 18, 2008 at 02:41:12PM +0200]
| Hi!
|
| > Fix potential NULL pointer dereference if kstrdup failed
| >
| > Signed-off-by: Cyrill Gorcunov
| >
| > ---
| >
| > Index: linux-2.6.git/drivers/misc/thinkpad_acpi.c
| > ================================================== =================
| > --- linux-2.6.git.orig/drivers/misc/thinkpad_acpi.c 2008-04-16 20:35:34.000000000 +0400
| > +++ linux-2.6.git/drivers/misc/thinkpad_acpi.c 2008-04-16 20:36:38.000000000 +0400
| > @@ -5826,7 +5826,7 @@ static void __init get_thinkpad_model_da
| >
| > tp->model_str = kstrdup(dmi_get_system_info(DMI_PRODUCT_VERSION),
| > GFP_KERNEL);
| > - if (strnicmp(tp->model_str, "ThinkPad", 8) != 0) {
| > + if (tp->model_str && strnicmp(tp->model_str, "ThinkPad", 8) != 0) {
| > kfree(tp->model_str);
| > tp->model_str = NULL;
| > }
|
| are you sure? This seems to assume machine is thinkpad if kstrdup
| fails... which is very wrong.
|

Oh, I see what do you mean - you mean that even if machine is ThinkPad
*but* kstrdup failed with my patch it would lead that the machine will
*not* be recognized as ThinkPad and that is not correct, agreed. But how
to preven from NULL dereference then? I think the current situation
brought by my patch would not lead to really critical problems *but*
it should be reorganized indeed! Thanks a lot, Pavel, for comments

- Cyrill -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Pavel Machek - Fri, Apr 18, 2008 at 02:41:12PM +0200]
| Hi!
|
| > Fix potential NULL pointer dereference if kstrdup failed
| >
| > Signed-off-by: Cyrill Gorcunov
| >
| > ---
| >
| > Index: linux-2.6.git/drivers/misc/thinkpad_acpi.c
| > ================================================== =================
| > --- linux-2.6.git.orig/drivers/misc/thinkpad_acpi.c 2008-04-16 20:35:34.000000000 +0400
| > +++ linux-2.6.git/drivers/misc/thinkpad_acpi.c 2008-04-16 20:36:38.000000000 +0400
| > @@ -5826,7 +5826,7 @@ static void __init get_thinkpad_model_da
| >
| > tp->model_str = kstrdup(dmi_get_system_info(DMI_PRODUCT_VERSION),
| > GFP_KERNEL);
| > - if (strnicmp(tp->model_str, "ThinkPad", 8) != 0) {
| > + if (tp->model_str && strnicmp(tp->model_str, "ThinkPad", 8) != 0) {
| > kfree(tp->model_str);
| > tp->model_str = NULL;
| > }
|
| are you sure? This seems to assume machine is thinkpad if kstrdup
| fails... which is very wrong.
|

Actually, my patch didn't bring any new into the current driver state,
just add additional check to prevent NULL deref, that's all, so I think
it's fine (but maybe additional printk with info would had been usefull).

- Cyrill -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, 18 Apr 2008, Cyrill Gorcunov wrote:
> Oh, I see what do you mean - you mean that even if machine is ThinkPad
> *but* kstrdup failed with my patch it would lead that the machine will
> *not* be recognized as ThinkPad and that is not correct, agreed. But how

That's acceptable. It is not worth bothering with this failure mode at
that point of the driver lifetime. Your patch makes it just keep
running, which is fine since if it can't kstrdup a small string, it will
abend with -ENOMEM soon enough when it tries to alocate other much
bigger structures.

Just in case, I will schedule a low-priority fix for later that will
-ENOMEM if kstrdup fails, but your patch is good enough a fix for now.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/