ThinkPad ACPI: fix possible NULL pointer dereference

PATCH In review Cyrill Gorcunov 4 participants 17 messages Apr 16, 2008 – Apr 19, 2008 1 message not in the archive

Subsystems (2)

NFS client Char and misc drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Cyrill Gorcunov

author

2008-04-16 17:48 UTC #1 lore raw

PATCH 1/3 NFS: fix potential NULL pointer dereference

It's possible to get NULL pointer dereference
if kstrndup failed

Here is a possible scenario

nfs4_get_sb
nfs4_validate_mount_data
o kstrndup failed so args->nfs_server.export_path = NULL
nfs4_create_server

    nfs4_path_walk(..., NULL) -> Oops!

Signed-off-by Cyrill Gorcunov <[email protected]>

---

fs/nfs/super.c +2 -0

Index: linux-2.6.git/fs/nfs/super.c
===================================================================
@@ -1858,6 +1858,8 @@ static int nfs4_validate_mount_data(void
 		if (len > NFS4_MAXPATHLEN)
 			return -ENAMETOOLONG;
 		args->nfs_server.export_path = kstrndup(c, len, GFP_KERNEL);
+		if (!args->nfs_server.export_path)
+			return -ENOMEM;
 
 		dprintk("NFS: MNTPATH: '%s'\n", args->nfs_server.export_path);

Trond Myklebust

2008-04-16 18:11 UTC #2 lore raw

Re: [patch 1/3] NFS: fix potential NULL pointer dereference

Quoted 6 lines · Cyrill Gorcunov

plain text document attachment (nfs-kstrdup-nul-fix) It's possible to get NULL pointer dereference if kstrndup failed
Here is a possible scenario

Quoted 7 lines

nfs4_get_sb
nfs4_validate_mount_data
o kstrndup failed so args->nfs_server.export_path = NULL
nfs4_create_server
    nfs4_path_walk(..., NULL) -> Oops!
Signed-off-by Cyrill Gorcunov <[email protected]>

Why fix only the one case? What about the other kstrdup/kstrndup cases in super.c that appear to be unchecked?

Trond

Quoted 15 lines

---

Index: linux-2.6.git/fs/nfs/super.c
===================================================================

fs/nfs/super.c +2 -0

@@ -1858,6 +1858,8 @@ static int nfs4_validate_mount_data(void
 		if (len > NFS4_MAXPATHLEN)
 			return -ENAMETOOLONG;
 		args->nfs_server.export_path = kstrndup(c, len, GFP_KERNEL);
+		if (!args->nfs_server.export_path)
+			return -ENOMEM;
 
 		dprintk("NFS: MNTPATH: '%s'\n", args->nfs_server.export_path);

Cyrill Gorcunov

author

2008-04-16 18:14 UTC #3 lore raw

Re: [patch 1/3] NFS: fix potential NULL pointer dereference

[Trond Myklebust - Wed, Apr 16, 2008 at 02:11:31PM -0400]

| 
| On Wed, 2008-04-16 at 21:44 +0400, Cyrill Gorcunov wrote:
| > plain text document attachment (nfs-kstrdup-nul-fix)
| > It's possible to get NULL pointer dereference
| > if kstrndup failed
| > 
| > Here is a possible scenario
| > 
| > nfs4_get_sb
| >   nfs4_validate_mount_data
| >     o kstrndup failed so args->nfs_server.export_path = NULL
| >   nfs4_create_server
| >     nfs4_path_walk(..., NULL) -> Oops!
| > 
| > Signed-off-by: Cyrill Gorcunov <[email protected]>
| 
| Why fix only the one case? What about the other kstrdup/kstrndup cases
| in super.c that appear to be unchecked?
| 
| Trond
| 
| > ---
| > 
| > Index: linux-2.6.git/fs/nfs/super.c
| > ===================================================================
| > --- linux-2.6.git.orig/fs/nfs/super.c	2008-04-15 23:01:30.000000000 +0400
| > +++ linux-2.6.git/fs/nfs/super.c	2008-04-16 20:01:44.000000000 +0400
| > @@ -1858,6 +1858,8 @@ static int nfs4_validate_mount_data(void
| >  		if (len > NFS4_MAXPATHLEN)
| >  			return -ENAMETOOLONG;
| >  		args->nfs_server.export_path = kstrndup(c, len, GFP_KERNEL);
| > +		if (!args->nfs_server.export_path)
| > +			return -ENOMEM;
| >  
| >  		dprintk("NFS: MNTPATH: '%s'\n", args->nfs_server.export_path);
| > 
|

This one is leading to NULL deref, others - don't

- Cyrill -

Trond Myklebust

2008-04-16 18:55 UTC #4 lore raw

Re: [patch 1/3] NFS: fix potential NULL pointer dereference

Quoted 39 lines · Cyrill Gorcunov

[Trond Myklebust - Wed, Apr 16, 2008 at 02:11:31PM -0400]

| 
| On Wed, 2008-04-16 at 21:44 +0400, Cyrill Gorcunov wrote:
| > plain text document attachment (nfs-kstrdup-nul-fix)
| > It's possible to get NULL pointer dereference
| > if kstrndup failed
| > 
| > Here is a possible scenario
| > 
| > nfs4_get_sb
| >   nfs4_validate_mount_data
| >     o kstrndup failed so args->nfs_server.export_path = NULL
| >   nfs4_create_server
| >     nfs4_path_walk(..., NULL) -> Oops!
| > 
| > Signed-off-by: Cyrill Gorcunov <[email protected]>
| 
| Why fix only the one case? What about the other kstrdup/kstrndup cases
| in super.c that appear to be unchecked?
| 
| Trond
| 
| > ---
| > 
| > Index: linux-2.6.git/fs/nfs/super.c
| > ===================================================================
| > --- linux-2.6.git.orig/fs/nfs/super.c	2008-04-15 23:01:30.000000000 +0400
| > +++ linux-2.6.git/fs/nfs/super.c	2008-04-16 20:01:44.000000000 +0400
| > @@ -1858,6 +1858,8 @@ static int nfs4_validate_mount_data(void
| >  		if (len > NFS4_MAXPATHLEN)
| >  			return -ENAMETOOLONG;
| >  		args->nfs_server.export_path = kstrndup(c, len, GFP_KERNEL);
| > +		if (!args->nfs_server.export_path)
| > +			return -ENOMEM;
| >  
| >  		dprintk("NFS: MNTPATH: '%s'\n", args->nfs_server.export_path);
| > 
|

Quoted 1 line

This one is leading to NULL deref, others - don't

So? The defensive coding principle is that you perform validity checks when the pointer is created. Otherwise, we could equally well have added the NULL deref check to nfs4_path_walk()...

Trond

Cyrill Gorcunov

author

2008-04-16 20:19 UTC #5 lore raw

Re: [patch 1/3] NFS: fix potential NULL pointer dereference

Trond, I've just pointed the problem and its solution (which is seems to be a bit ugly, according to the rest nfs coding principle). So if you prefer to have such a check in 'walk_path' function - just say me that. You choose :) Thanks for comments

Quoted 51 lines · Trond Myklebust

Quoted 19 lines · Cyrill Gorcunov

[Trond Myklebust - Wed, Apr 16, 2008 at 02:11:31PM -0400]

|
| On Wed, 2008-04-16 at 21:44 +0400, Cyrill Gorcunov wrote:
| > plain text document attachment (nfs-kstrdup-nul-fix)
| > It's possible to get NULL pointer dereference
| > if kstrndup failed
| >
| > Here is a possible scenario
| >
| > nfs4_get_sb
| >   nfs4_validate_mount_data
| >     o kstrndup failed so args->nfs_server.export_path = NULL
| >   nfs4_create_server
| >     nfs4_path_walk(..., NULL) -> Oops!
| >
| > Signed-off-by: Cyrill Gorcunov <[email protected]>
|
| Why fix only the one case? What about the other kstrdup/kstrndup cases
| in super.c that appear to be unchecked?

Quoted 8 lines

|
| Trond
|
| > ---
| >
| > Index: linux-2.6.git/fs/nfs/super.c
| > ===================================================================
| > --- linux-2.6.git.orig/fs/nfs/super.c	2008-04-15 23:01:30.000000000

+0400

Quoted 12 lines

| > +++ linux-2.6.git/fs/nfs/super.c	2008-04-16 20:01:44.000000000 +0400
| > @@ -1858,6 +1858,8 @@ static int nfs4_validate_mount_data(void
| >  		if (len > NFS4_MAXPATHLEN)
| >  			return -ENAMETOOLONG;
| >  		args->nfs_server.export_path = kstrndup(c, len, GFP_KERNEL);
| > +		if (!args->nfs_server.export_path)
| > +			return -ENOMEM;
| >
| >  		dprintk("NFS: MNTPATH: '%s'\n", args->nfs_server.export_path);
| >
|

Quoted 1 line

This one is leading to NULL deref, others - don't

So? The defensive coding principle is that you perform validity checks when the pointer is created. Otherwise, we could equally well have added the NULL deref check to nfs4_path_walk()...

Trond

Trond Myklebust

2008-04-16 20:40 UTC #6 lore raw

Re: [patch 1/3] NFS: fix potential NULL pointer dereference

Quoted 8 lines · Cyrill Gorcunov

Trond, I've just pointed the problem and its solution (which is seems to be a bit ugly, according to the rest nfs coding principle). So if you prefer to have such a check in 'walk_path' function - just say me that. You choose :) Thanks for comments
Quoted 3 lines
So? The defensive coding principle is that you perform validity checks when the pointer is created. Otherwise, we could equally well have added the NULL deref check to nfs4_path_walk()...

No, your fix was correct, it was just incomplete.

The point I was making above was that defensive programming means that _all_ these validity/NULL pointer checks should really be done in nfs4_validate_mount_data and nfs_validate_mount_data. We shouldn't rely on checks in other parts of the code.

In fact, as an example: it looks to me as if the lack of a nfs_server.hostname, leads to a lack of nfs_client->cl_hostname, which will eventually cause an Oops if you 'cat /proc/fs/nfsfs/servers', or if you hit the printk in nfs_update_inode(), or various other dprintk()s.

Trond

Cyrill Gorcunov

author

2008-04-17 04:25 UTC #7 lore raw

Re: [patch 1/3] NFS: fix potential NULL pointer dereference

Quoted 19 lines · Trond Myklebust

Quoted 9 lines · Cyrill Gorcunov
Trond, I've just pointed the problem and its solution (which is seems to be a bit ugly, according to the rest nfs coding principle). So if you prefer to have such a check in 'walk_path' function - just say me that. You choose :) Thanks for comments
Quoted 3 lines
So? The defensive coding principle is that you perform validity checks when the pointer is created. Otherwise, we could equally well have added the NULL deref check to nfs4_path_walk()...
No, your fix was correct, it was just incomplete.
The point I was making above was that defensive programming means that _all_ these validity/NULL pointer checks should really be done in nfs4_validate_mount_data and nfs_validate_mount_data. We shouldn't rely on checks in other parts of the code.

Quoted 8 lines

In fact, as an example: it looks to me as if the lack of a nfs_server.hostname, leads to a lack of nfs_client->cl_hostname, which will eventually cause an Oops if you 'cat /proc/fs/nfsfs/servers', or if you hit the printk in nfs_update_inode(), or various other dprintk()s.
Trond

Thanks Trond, I'll remake it ASAP (but can't guarantie that it will be soon ;)

Cyrill Gorcunov

author

2008-04-17 07:25 UTC #8 lore raw

Re: [patch 1/3] NFS: fix potential NULL pointer dereference

Quoted 29 lines · Cyrill Gorcunov

On Thu, Apr 17, 2008 at 12:40 AM, Trond Myklebust
Quoted 26 lines · <[email protected]> wrote:
Quoted 9 lines · Cyrill Gorcunov
Trond, I've just pointed the problem and its solution (which is seems to be a bit ugly, according to the rest nfs coding principle). So if you prefer to have such a check in 'walk_path' function - just say me that. You choose :) Thanks for comments
Quoted 3 lines
So? The defensive coding principle is that you perform validity checks when the pointer is created. Otherwise, we could equally well have added the NULL deref check to nfs4_path_walk()...
No, your fix was correct, it was just incomplete.
The point I was making above was that defensive programming means that _all_ these validity/NULL pointer checks should really be done in nfs4_validate_mount_data and nfs_validate_mount_data. We shouldn't rely on checks in other parts of the code.
In fact, as an example: it looks to me as if the lack of a nfs_server.hostname, leads to a lack of nfs_client->cl_hostname, which will eventually cause an Oops if you 'cat /proc/fs/nfsfs/servers', or if you hit the printk in nfs_update_inode(), or various other dprintk()s.

Quoted 6 lines

Quoted 3 lines
Trond
Thanks Trond, I'll remake it ASAP (but can't guarantie that it will be soon ;)

Hi Trond,

here is an updated version enveloped (can't send it by inline 'cause I'm in office now and have to use Web interface to my mail)

Please review (any comments are welcome - as always ;)

From: Cyrill Gorcunov <[email protected]>
Subject: [PATCH] NFS - fix possible NULL pointer dereference

kstrndup and kstrdup may return NULL so we should be ready for a such situation

Signed-off-by Cyrill Gorcunov <[email protected]>

---

fs/nfs/super.c +16 -1

@@ -1211,6 +1211,8 @@ static int nfs_validate_mount_data(void 
 			args->nfs_server.protocol = XPRT_TRANSPORT_UDP;
 		/* N.B. caller will free nfs_server.hostname in all cases */
 		args->nfs_server.hostname = kstrdup(data->hostname, GFP_KERNEL);
+		if (!args->nfs_server.hostname)
+			goto out_nomem;
 		args->namlen		= data->namlen;
 		args->bsize		= data->bsize;
 		args->auth_flavors[0]	= data->pseudoflavor;

@@ -1233,6 +1235,8 @@ static int nfs_validate_mount_data(void 
 		len = c - dev_name;
 		/* N.B. caller will free nfs_server.hostname in all cases */
 		args->nfs_server.hostname = kstrndup(dev_name, len, GFP_KERNEL);
+		if (!args->nfs_server.hostname)
+			goto out_nomem;
 
 		c++;
 		if (strlen(c) > NFS_MAXPATHLEN)

@@ -1280,6 +1284,10 @@ out_no_address:
 	dfprintk(MOUNT, "NFS: mount program didn't pass remote address\n");
 	return -EINVAL;
 
+out_nomem:
+	dfprintk(MOUNT, "NFS: not enough memory to duplicate string\n");
+	return -ENOMEM;
+
 out_invalid_fh:
 	dfprintk(MOUNT, "NFS: invalid root filehandle\n");
 	return -EINVAL;

@@ -1797,12 +1805,15 @@ static int nfs4_validate_mount_data(void
 			return -ENAMETOOLONG;
 		/* N.B. caller will free nfs_server.hostname in all cases */
 		args->nfs_server.hostname = kstrndup(dev_name, len, GFP_KERNEL);
-
+		if (!args->nfs_server.hostname)
+			goto out_nomem;
 		c++;			/* step over the ':' */
 		len = strlen(c);
 		if (len > NFS4_MAXPATHLEN)
 			return -ENAMETOOLONG;
 		args->nfs_server.export_path = kstrndup(c, len, GFP_KERNEL);
+		if (!args->nfs_server.export_path)
+			goto out_nomem;
 
 		dprintk("NFS: MNTPATH: '%s'\n", args->nfs_server.export_path);

@@ -1827,6 +1838,10 @@ out_inval_auth:
 out_no_address:
 	dfprintk(MOUNT, "NFS4: mount program didn't pass remote address\n");
 	return -EINVAL;
+
+out_nomem:
+	dfprintk(MOUNT, "NFS4: not enough memory to duplicate string\n");
+	return -ENOMEM;
 
 out_no_client_address:
 	dfprintk(MOUNT, "NFS4: mount program didn't pass callback address\n");

Cyrill Gorcunov

author

2008-04-16 20:24 UTC #9 lore raw

Re: [patch 1/3] NFS: fix potential NULL pointer dereference

In reply to Trond Myklebust

Btw, may be you meant to perform 'near' check for rest of nfs code? But that wold requare much more code to change...

Quoted 51 lines · Trond Myklebust

Quoted 19 lines · Cyrill Gorcunov

[Trond Myklebust - Wed, Apr 16, 2008 at 02:11:31PM -0400]

|
| On Wed, 2008-04-16 at 21:44 +0400, Cyrill Gorcunov wrote:
| > plain text document attachment (nfs-kstrdup-nul-fix)
| > It's possible to get NULL pointer dereference
| > if kstrndup failed
| >
| > Here is a possible scenario
| >
| > nfs4_get_sb
| >   nfs4_validate_mount_data
| >     o kstrndup failed so args->nfs_server.export_path = NULL
| >   nfs4_create_server
| >     nfs4_path_walk(..., NULL) -> Oops!
| >
| > Signed-off-by: Cyrill Gorcunov <[email protected]>
|
| Why fix only the one case? What about the other kstrdup/kstrndup cases
| in super.c that appear to be unchecked?

Quoted 8 lines

|
| Trond
|
| > ---
| >
| > Index: linux-2.6.git/fs/nfs/super.c
| > ===================================================================
| > --- linux-2.6.git.orig/fs/nfs/super.c	2008-04-15 23:01:30.000000000

+0400

Quoted 12 lines

| > +++ linux-2.6.git/fs/nfs/super.c	2008-04-16 20:01:44.000000000 +0400
| > @@ -1858,6 +1858,8 @@ static int nfs4_validate_mount_data(void
| >  		if (len > NFS4_MAXPATHLEN)
| >  			return -ENAMETOOLONG;
| >  		args->nfs_server.export_path = kstrndup(c, len, GFP_KERNEL);
| > +		if (!args->nfs_server.export_path)
| > +			return -ENOMEM;
| >
| >  		dprintk("NFS: MNTPATH: '%s'\n", args->nfs_server.export_path);
| >
|

Quoted 1 line

This one is leading to NULL deref, others - don't

So? The defensive coding principle is that you perform validity checks when the pointer is created. Otherwise, we could equally well have added the NULL deref check to nfs4_path_walk()...

Trond

Cyrill Gorcunov

author

2008-04-16 17:48 UTC #10 lore raw

PATCH 2/3 CAPIFS: fix memory leak on remount

In reply to a message not in the archive

capifs_remount may reach 'return' statement without freeing of memory allocated by kstrdup call

Signed-off-by Cyrill Gorcunov <[email protected]>

---

drivers/isdn/capi/capifs.c +1 -0

Index: linux-2.6.git/drivers/isdn/capi/capifs.c
===================================================================
@@ -69,6 +69,7 @@ static int capifs_remount(struct super_b
 		} else if (sscanf(this_char, "mode=%o%c", &n, &dummy) == 1)
 			mode = n & ~S_IFMT;
 		else {
+			kfree(new_opt);
 			printk("capifs: called with bogus options\n");
 			return -EINVAL;
 		}

Cyrill Gorcunov

author

2008-04-16 17:48 UTC #11 lore raw

PATCH 3/3 ThinkPad ACPI: fix possible NULL pointer dereference

In reply to a message not in the archive

Fix potential NULL pointer dereference if kstrdup failed

Signed-off-by Cyrill Gorcunov <[email protected]>

---

drivers/misc/thinkpad_acpi.c +1 -1

Index: linux-2.6.git/drivers/misc/thinkpad_acpi.c
===================================================================
@@ -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;
 	}

Henrique de Moraes Holschuh

Acked-by on v1 3/3

2008-04-16 20:53 UTC #12 lore raw

Quoted 3 lines · Cyrill Gorcunov

Fix potential NULL pointer dereference if kstrdup failed
Signed-off-by Cyrill Gorcunov <[email protected]>

Acked-by Henrique de Moraes Holschuh <[email protected]>

Quoted 15 lines

Index: linux-2.6.git/drivers/misc/thinkpad_acpi.c
===================================================================

drivers/misc/thinkpad_acpi.c +1 -1

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

Signature · 1 line

--

Signature · 4 lines

  "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

Pavel Machek

2008-04-18 12:41 UTC #13 lore raw

In reply to Cyrill Gorcunov

Hi!

Quoted 11 lines

Fix potential NULL pointer dereference if kstrdup failed
Signed-off-by Cyrill Gorcunov <[email protected]>
---
Index: linux-2.6.git/drivers/misc/thinkpad_acpi.c
===================================================================

drivers/misc/thinkpad_acpi.c static void __init get_thinkpad_model_da · line 5832

Quoted 8 lines

drivers/misc/thinkpad_acpi.c +1 -1

@@ -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.

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Cyrill Gorcunov

author

2008-04-18 12:55 UTC #14 lore raw

[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 <[email protected]>
| > 
| > ---
| > 
| > 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 -

Cyrill Gorcunov

author

2008-04-18 13:08 UTC #15 lore raw

In reply to Pavel Machek

[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 <[email protected]>
| > 
| > ---
| > 
| > 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 -

Henrique de Moraes Holschuh

2008-04-19 04:10 UTC #16 lore raw

Quoted 3 lines · Cyrill Gorcunov

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.

Signature · 4 lines

  "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

Cyrill Gorcunov

author

2008-04-18 13:54 UTC #17 lore raw

In reply to Pavel Machek

[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 <[email protected]>
| > 
| > ---
| > 
| > 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 -