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

This is a discussion on [patch 1/3] NFS: fix potential NULL pointer dereference - Kernel ; 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 --- Index: linux-2.6.git/fs/nfs/super.c ================================================== ================= --- linux-2.6.git.orig/fs/nfs/super.c 2008-04-15 ...

+ Reply to Thread
Results 1 to 8 of 8

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

  1. [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

    ---

    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@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 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
    |
    | 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@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 1/3] NFS: fix potential NULL pointer dereference


    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


    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@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 1/3] NFS: fix potential NULL pointer dereference


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

    On 4/16/08, Trond Myklebust 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
    > > |
    > > | 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@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 1/3] NFS: fix potential NULL pointer dereference

    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 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
    > > |
    > > | 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@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 1/3] NFS: fix potential NULL pointer dereference


    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Thu, Apr 17, 2008 at 12:40 AM, Trond Myklebust
    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@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