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

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

[Cyrill Gorcunov]

[-- Attachment #1: nfs-kstrdup-nul-fix --]
[-- Type: text/plain, Size: 875 bytes --]

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 <gorcunov@gmail.com>

---

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

-- 

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

[Trond Myklebust]

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 <gorcunov@gmail.com>

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

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

[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 <gorcunov@gmail.com>
| 
| 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 -

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

[Trond Myklebust]

On Wed, 2008-04-16 at 22:13 +0400, Cyrill Gorcunov wrote:
> [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 <gorcunov@gmail.com>
> | 
> | 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

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

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

[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

On 4/16/08, Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
>
> On Wed, 2008-04-16 at 22:13 +0400, Cyrill Gorcunov wrote:
> > [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 <gorcunov@gmail.com>
> > |
> > | 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
>
> 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
>
>

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

[Trond Myklebust]

On Thu, 2008-04-17 at 00:19 +0400, Cyrill Gorcunov wrote:
> 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

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

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

[Cyrill Gorcunov]

On Thu, Apr 17, 2008 at 12:40 AM, Trond Myklebust
<trond.myklebust@fys.uio.no> wrote:
>
>  On Thu, 2008-04-17 at 00:19 +0400, Cyrill Gorcunov wrote:
>  > 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
>
>
> > > 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
>
>

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

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

[Cyrill Gorcunov]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

On Thu, Apr 17, 2008 at 8:25 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Thu, Apr 17, 2008 at 12:40 AM, Trond Myklebust
>
> <trond.myklebust@fys.uio.no> wrote:
>  >
>
>
> >  On Thu, 2008-04-17 at 00:19 +0400, Cyrill Gorcunov wrote:
>  >  > 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
>  >
>  >
>  > > > 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
>  >
>  >
>
>  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 ;)

[-- Attachment #2: super.diff --]
[-- Type: application/octet-stream, Size: 2309 bytes --]

From: Cyrill Gorcunov <gorcunov@gmail.com>
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 <gorcunov@gmail.com>

---

--- a/fs/nfs/super.c	Wed Mar 05 07:33:54 2008
+++ a/fs/nfs/super.c	Thu Apr 17 11:10:34 2008
@@ -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");

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

[Cyrill Gorcunov]

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

On 4/16/08, Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
>
> On Wed, 2008-04-16 at 22:13 +0400, Cyrill Gorcunov wrote:
> > [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 <gorcunov@gmail.com>
> > |
> > | 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
>
> 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
>
>

[patch 2/3] CAPIFS: fix memory leak on remount

[Cyrill Gorcunov]

[-- Attachment #1: capi-memory-leak --]
[-- Type: text/plain, Size: 695 bytes --]

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

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

---

Index: linux-2.6.git/drivers/isdn/capi/capifs.c
===================================================================
--- linux-2.6.git.orig/drivers/isdn/capi/capifs.c	2008-04-16 20:14:46.000000000 +0400
+++ linux-2.6.git/drivers/isdn/capi/capifs.c	2008-04-16 20:16:58.000000000 +0400
@@ -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;
 		}

-- 

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

[Cyrill Gorcunov]

[-- Attachment #1: thinkpad-acpi-null-fix --]
[-- Type: text/plain, Size: 739 bytes --]

Fix potential NULL pointer dereference if kstrdup failed

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

---

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

-- 

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

[Henrique de Moraes Holschuh]

On Wed, 16 Apr 2008, Cyrill Gorcunov wrote:
> Fix potential NULL pointer dereference if kstrdup failed
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

> 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

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

[Pavel Machek]

Hi!

> Fix potential NULL pointer dereference if kstrdup failed
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> ---
> 
> 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/~pavel/picture/horses/blog.html

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

[Cyrill Gorcunov]

[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 <gorcunov@gmail.com>
| > 
| > ---
| > 
| > 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 -

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

[Cyrill Gorcunov]

[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 <gorcunov@gmail.com>
| > 
| > ---
| > 
| > 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 -

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

[Henrique de Moraes Holschuh]

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

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

[Cyrill Gorcunov]

[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 <gorcunov@gmail.com>
| > 
| > ---
| > 
| > 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 -