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 10:06 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> 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>

And it's not 4.9 stable its 4.10 fixes of course

@stable, sorry about this noise

>> 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 linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux