[PATCH] NFS: Remove superfluous kmap in nfs_readdir_xdr_to_array

[PATCH] NFS: Remove superfluous kmap in nfs_readdir_xdr_to_array

[Petr Malat]

Array is mapped by nfs_readdir_get_array(), the further kmap is a result
of a bad merge and should be removed.

This resource leakage can be exploited for DoS by receptively reading
a content of a directory on NFS (e.g. by running ls).

Fixes: 67a56e9743171 ("NFS: Fix memory leaks and corruption in readdir")
Signed-off-by: Petr Malat <oss@malat.biz>
---
 fs/nfs/dir.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index c2665d920cf8..2517fcd423b6 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -678,8 +678,6 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
 		goto out_label_free;
 	}
 
-	array = kmap(page);
-
 	status = nfs_readdir_alloc_pages(pages, array_size);
 	if (status < 0)
 		goto out_release_array;
-- 
2.20.1

Re: [PATCH] NFS: Remove superfluous kmap in nfs_readdir_xdr_to_array

[Linus Torvalds]

Adding more people.

The old stable trees seem to have rather different code.

[ Goes off and looks at the stable trees ]

Petr seems entirely correct - the stable tree backport appears broken.

Because looking at that commit 67a56e9743171 in the stable tree, it
doesn't seem to match commit 4b310319c6a8 ("NFS: Fix memory leaks and
corruption in readdir") in mainline.

That stable backport looks bogus. It added that

        array = kmap(page);

line from somewhere else, probably because the stable tree didn't have
the line at all, and it was there in the context.

Because while mainline has that line to initialize array with kmap(),
in those stable trees, we have

        array = nfs_readdir_get_array(page);

and as Petr says, the kmap has been done there already, and it will be
kunmap'ed by nfs_readdir_release_array().

And looking closer, this same bug seems to have happened twice: it
also exists in 0b0223f9c3a8.

But somebody else should double-check me - somebody who actually knows the code.

As to how I found the other case, do this in the stable git repo with
all the stable tags:

    git log -p --no-merges --all \
        --grep="NFS: Fix memory leaks and corruption in readdir"

to see all the copies of that commit backport.

Add a

    -S'kmap(page)'

to that line to see the cases that added that line. Or to just get the commits:

    git log --oneline --no-merges --all \
        --grep="NFS: Fix memory leaks and corruption in readdir" \
        -S'kmap(page)'

and the result is

    67a56e974317 NFS: Fix memory leaks and corruption in readdir
    0b0223f9c3a8 NFS: Fix memory leaks and corruption in readdir

Those two seem to be the 4.4 and 4.9 backports:

    git name-rev 67a56e974317 0b0223f9c3a8

gives:

    67a56e974317 tags/v4.9.214~38
    0b0223f9c3a8 tags/v4.4.214~31

and I guess that makes sense - they are the really old ones.

             Linus

On Fri, Mar 13, 2020 at 1:25 PM Petr Malat <oss@malat.biz> wrote:
>
> Array is mapped by nfs_readdir_get_array(), the further kmap is a result
> of a bad merge and should be removed.
>
> This resource leakage can be exploited for DoS by receptively reading
> a content of a directory on NFS (e.g. by running ls).
>
> Fixes: 67a56e9743171 ("NFS: Fix memory leaks and corruption in readdir")
> Signed-off-by: Petr Malat <oss@malat.biz>
> ---
>  fs/nfs/dir.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index c2665d920cf8..2517fcd423b6 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -678,8 +678,6 @@ int nfs_readdir_xdr_to_array(nfs_readdir_descriptor_t *desc, struct page *page,
>                 goto out_label_free;
>         }
>
> -       array = kmap(page);
> -
>         status = nfs_readdir_alloc_pages(pages, array_size);
>         if (status < 0)
>                 goto out_release_array;
> --
> 2.20.1
>

Re: [PATCH] NFS: Remove superfluous kmap in nfs_readdir_xdr_to_array

[Sasha Levin]

On Fri, Mar 13, 2020 at 02:05:27PM -0700, Linus Torvalds wrote:
>Adding more people.
>
>The old stable trees seem to have rather different code.
>
>[ Goes off and looks at the stable trees ]
>
>Petr seems entirely correct - the stable tree backport appears broken.
>
>Because looking at that commit 67a56e9743171 in the stable tree, it
>doesn't seem to match commit 4b310319c6a8 ("NFS: Fix memory leaks and
>corruption in readdir") in mainline.
>
>That stable backport looks bogus. It added that
>
>        array = kmap(page);
>
>line from somewhere else, probably because the stable tree didn't have
>the line at all, and it was there in the context.

I botched up that backport, sorry.

>Because while mainline has that line to initialize array with kmap(),
>in those stable trees, we have
>
>        array = nfs_readdir_get_array(page);
>
>and as Petr says, the kmap has been done there already, and it will be
>kunmap'ed by nfs_readdir_release_array().
>
>And looking closer, this same bug seems to have happened twice: it
>also exists in 0b0223f9c3a8.
>
>But somebody else should double-check me - somebody who actually knows the code.
>
>As to how I found the other case, do this in the stable git repo with
>all the stable tags:
>
>    git log -p --no-merges --all \
>        --grep="NFS: Fix memory leaks and corruption in readdir"
>
>to see all the copies of that commit backport.
>
>Add a
>
>    -S'kmap(page)'
>
>to that line to see the cases that added that line. Or to just get the commits:
>
>    git log --oneline --no-merges --all \
>        --grep="NFS: Fix memory leaks and corruption in readdir" \
>        -S'kmap(page)'
>
>and the result is
>
>    67a56e974317 NFS: Fix memory leaks and corruption in readdir
>    0b0223f9c3a8 NFS: Fix memory leaks and corruption in readdir

I've applied to fix to the 4.9 and 4.4 trees, thank you!

-- 
Thanks,
Sasha