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

["Cyrill Gorcunov" <gorcunov@gmail.com>]

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