Re: [PATCH] ovl: fix possible use after free on redirect dir lookup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 17, 2017 at 2:34 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> ovl_lookup_layer() iterates on path elements of d->name.name
> but also frees and allocates a new pointer for d->name.name.
>
> For the case of lookup in upper layer, the initial d->name.name
> pointer is stable (dentry->d_name), but for lower layers, the
> initial d->name.name can be d->redirect, which can be freed during
> iteration.
>
> Make a copy of initial d->name.name before iterating it and reset
> the iteration pointer to point inside the newly allocated d->redirect
> path after lookup of every element in the path.
>
> cc: stable [v4.9] <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
>
> Miklos,
>
> I have not written a test case to prove this bug yet, just had
> a WTF moment while working on redirect_fh.
>
> Am I missing something??
>
> Will try to add a test case to reproduce.
>

<sigh> I added some test cases and even improved the recycling infra:
https://github.com/amir73il/unionmount-testsuite/commits/overlayfs-devel
but all I could get was a proof by adding another WARN_ON() in the code
and print the garbage pointer.
Breaking lookup itself proved to be harder, so I stopped trying after I got
the evidence I needed:

[  148.538035] ---[ end trace 8aa5aa8192a9dcf5 ]---
[  148.538037] ovl_lookup_layer: next =
'kkkkkkkkkkkkkkkkkkkkk\xffffffa57:131', postlen = 0, name =
'/a/dir105'
[  148.613920] ./run --ov --ts=0 rename-move-dir
[  148.684563] TEST rename-move-dir.py:10: Move empty dir into another
[  148.693472] TEST rename-move-dir.py:23: Move populated dir into another
[  148.704143] TEST rename-move-dir.py:37: Rename populated dir and
move into another
[  148.720317] TEST rename-move-dir.py:55: Move populated dir into
another and rename
[  148.735241] TEST rename-move-dir.py:73: Move populated dir into
another and move subdir into another
[  148.755020] TEST rename-move-dir.py:95: Rename populated dir and
parent and move into another

As a result on running ./run --ov=10 rename-move-dir with new tests
and with the additional
WARN_ON() and pr_err():

index 9ad48d9..4ca63be 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -165,6 +165,7 @@ static int ovl_lookup_layer(struct dentry *base,
struct ovl_lookup_data *d,
        while (*s++ == '/' && !IS_ERR_OR_NULL(base) && d_can_lookup(base)) {
                const char *next = strchrnul(s, '/');
                size_t slen = strlen(s);
+               size_t postlen = slen - (next - s);

                if (WARN_ON(slen > d->name.len) ||
                    WARN_ON(strcmp(d->name.name + d->name.len - slen, s)))
@@ -177,6 +178,11 @@ static int ovl_lookup_layer(struct dentry *base,
struct ovl_lookup_data *d,
                        return err;
                dentry = base;
                s = next;
+
+               if (WARN_ON(strlen(next) != postlen)) {
+                       pr_err("%s: next = '%s', postlen = %ld, name =
'%s'\n", __func__, next, postlen, d->name.name);
+                       //return -EIO;
+               }
        }
        *ret = dentry;
        return 0;



> Amir.
>
>  fs/overlayfs/namei.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 9ad48d9..9cb589c 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -154,6 +154,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
>  static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
>                             struct dentry **ret)
>  {
> +       struct qstr name = d->name;
>         const char *s = d->name.name;
>         struct dentry *dentry = NULL;
>         int err;
> @@ -162,24 +163,36 @@ static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
>                 return ovl_lookup_single(base, d, d->name.name, d->name.len,
>                                          0, "", ret);
>
> +       /* ovl_lookup_single() -> ovl_check_redirect() may free d->name.name */
> +       s = name.name = kstrdup(d->name.name, GFP_KERNEL);
> +       if (!s)
> +               return -ENOMEM;
> +
>         while (*s++ == '/' && !IS_ERR_OR_NULL(base) && d_can_lookup(base)) {
> -               const char *next = strchrnul(s, '/');
> +               const char *post = strchrnul(s, '/');
>                 size_t slen = strlen(s);
> +               size_t postlen = slen - (post - s);
>
> -               if (WARN_ON(slen > d->name.len) ||
> -                   WARN_ON(strcmp(d->name.name + d->name.len - slen, s)))
> -                       return -EIO;
> -
> -               err = ovl_lookup_single(base, d, s, next - s,
> -                                       d->name.len - slen, next, &base);
> +               err = ovl_lookup_single(base, d, s, post - s,
> +                                       d->name.len - slen, post, &base);
>                 dput(dentry);
>                 if (err)
> -                       return err;
> +                       goto out_err;
>                 dentry = base;
> -               s = next;
> +
> +               /* d->name.name may be a new string with modified prefix */
> +               s = d->name.name + d->name.len - postlen;
> +               err = -EIO;
> +               if (WARN_ON(postlen > name.len) ||
> +                   WARN_ON(strcmp(name.name + name.len - postlen, s)))
> +                       goto out_err;
>         }
> +
>         *ret = dentry;
> -       return 0;
> +       err = 0;
> +out_err:
> +       kfree(name.name);
> +       return err;
>  }
>
>  /*
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]