[patch 3/3] ThinkPad ACPI: fix possible NULL pointer dereference - Kernel

This is a discussion on [patch 3/3] ThinkPad ACPI: fix possible NULL pointer dereference - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 7 of 7

Thread: [patch 3/3] ThinkPad ACPI: fix possible NULL pointer dereference

  1. [patch 3/3] ThinkPad ACPI: fix possible NULL pointer dereference

    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;
    }

    --
    --
    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/

  2. Re: [patch 3/3] ThinkPad ACPI: fix possible NULL pointer dereference

    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/

  3. Re: [patch 3/3] ThinkPad ACPI: fix possible NULL pointer dereference

    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/

  4. Re: [patch 3/3] ThinkPad ACPI: fix possible NULL pointer dereference

    [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/

  5. Re: [patch 3/3] ThinkPad ACPI: fix possible NULL pointer dereference

    [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/

  6. Re: [patch 3/3] ThinkPad ACPI: fix possible NULL pointer dereference

    [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/

  7. Re: [patch 3/3] ThinkPad ACPI: fix possible NULL pointer dereference

    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/

+ Reply to Thread