Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

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

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


gorcunov at gmail

Apr 16, 2008, 10:44 AM

Post #1 of 9 (1981 views)
Permalink
[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 <gorcunov [at] gmail>

---

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

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


trond.myklebust at fys

Apr 16, 2008, 11:11 AM

Post #2 of 9 (1925 views)
Permalink
Re: [patch 1/3] NFS: fix potential NULL pointer dereference [In reply to]

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 [at] gmail>

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

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


gorcunov at gmail

Apr 16, 2008, 11:13 AM

Post #3 of 9 (1936 views)
Permalink
Re: [patch 1/3] NFS: fix potential NULL pointer dereference [In reply to]

[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 [at] gmail>
|
| 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 -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


trond.myklebust at fys

Apr 16, 2008, 11:55 AM

Post #4 of 9 (1916 views)
Permalink
Re: [patch 1/3] NFS: fix potential NULL pointer dereference [In reply to]

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 [at] gmail>
> |
> | 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

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


gorcunov at gmail

Apr 16, 2008, 1:19 PM

Post #5 of 9 (1918 views)
Permalink
Re: [patch 1/3] NFS: fix potential NULL pointer dereference [In reply to]

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 [at] fys> 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 [at] gmail>
> > |
> > | 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gorcunov at gmail

Apr 16, 2008, 1:24 PM

Post #6 of 9 (1918 views)
Permalink
Re: [patch 1/3] NFS: fix potential NULL pointer dereference [In reply to]

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 [at] fys> 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 [at] gmail>
> > |
> > | 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


trond.myklebust at fys

Apr 16, 2008, 1:40 PM

Post #7 of 9 (1914 views)
Permalink
Re: [patch 1/3] NFS: fix potential NULL pointer dereference [In reply to]

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

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


gorcunov at gmail

Apr 16, 2008, 9:25 PM

Post #8 of 9 (1905 views)
Permalink
Re: [patch 1/3] NFS: fix potential NULL pointer dereference [In reply to]

On Thu, Apr 17, 2008 at 12:40 AM, Trond Myklebust
<trond.myklebust [at] fys> 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 ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gorcunov at gmail

Apr 17, 2008, 12:25 AM

Post #9 of 9 (1909 views)
Permalink
Re: [patch 1/3] NFS: fix potential NULL pointer dereference [In reply to]

On Thu, Apr 17, 2008 at 8:25 AM, Cyrill Gorcunov <gorcunov [at] gmail> wrote:
> On Thu, Apr 17, 2008 at 12:40 AM, Trond Myklebust
>
> <trond.myklebust [at] fys> 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 ;)
Attachments: super.diff (2.25 KB)

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.