On Wed, Jan 18, 2017 at 12:38 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> Amir,
>
> Thanks for spotting this bug and for the patch.
>
> I simplified it a bit (appended)
>
Simpler indeed.
> I don't think it's justified to copy the string just for the sake of an
> assertation, so got rid of that. Also tweaked the breakout condition to make
> things clearer.
>
I agree that copying the string is an overkill, but if this bug has
taught us anything
is that the assertion was not sufficient, so I don't think we should
leave no assert
at all. please see my suggested assert below (with which I tested your patch).
> Only compile tested. Please let me know if this works or not.
>
It works.
> Thanks,
> Miklos
>
> ---
> From: Amir Goldstein <amir73il@xxxxxxxxx>
> Subject: ovl: fix possible use after free on redirect dir lookup
>
> 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.
>
> [SzM]
> Keep the count of remaining characters in the redirect path and calculate
> the current position from that. This works becuase only the prefix is
> modified, the ending always stays the same.
>
> Fixes: 02b69b284cd7 ("ovl: lookup redirects")
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> ---
> fs/overlayfs/namei.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -154,29 +154,32 @@ static int ovl_lookup_single(struct dent
> static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
> struct dentry **ret)
> {
> - const char *s = d->name.name;
> + /* Counting down from the end, since the prefix can change */
> + size_t rem = d->name.len;
> struct dentry *dentry = NULL;
> int err;
>
> - if (*s != '/')
> + if (d->name.name[0] != '/')
> return ovl_lookup_single(base, d, d->name.name, d->name.len,
> 0, "", ret);
>
> - while (*s++ == '/' && !IS_ERR_OR_NULL(base) && d_can_lookup(base)) {
> + rem--;
> + while (!IS_ERR_OR_NULL(base) && d_can_lookup(base)) {
> + const char *s = d->name.name + d->name.len - rem;
> const char *next = strchrnul(s, '/');
> - size_t slen = strlen(s);
> + size_t thislen = next - s;
> + bool end = !next[0];
>
> - 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);
+ /* Verify we did not go off the rails */
+ if (WARN_ON(s[-1] != '/'))
+ return -EIO;
+
> + err = ovl_lookup_single(base, d, s, thislen,
> + d->name.len - rem, next, &base);
> dput(dentry);
> if (err)
> return err;
> dentry = base;
> - s = next;
> + if (end)
> + break;
> +
> + rem -= thislen + 1;
> }
> *ret = dentry;
> return 0;
--
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
