* [PATCH] ovl: fix possible use after free on redirect dir lookup @ 2017-01-17 12:34 Amir Goldstein 2017-01-17 20:06 ` Amir Goldstein 2017-01-18 8:15 ` [PATCH v2] " Amir Goldstein 0 siblings, 2 replies; 7+ messages in thread From: Amir Goldstein @ 2017-01-17 12:34 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-unionfs, stable [v4.9] 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@vger.kernel.org> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- 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. 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ovl: fix possible use after free on redirect dir lookup 2017-01-17 12:34 [PATCH] ovl: fix possible use after free on redirect dir lookup Amir Goldstein @ 2017-01-17 20:06 ` Amir Goldstein 2017-01-18 4:46 ` Amir Goldstein 2017-01-18 8:15 ` [PATCH v2] " Amir Goldstein 1 sibling, 1 reply; 7+ messages in thread From: Amir Goldstein @ 2017-01-17 20:06 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-unionfs, stable [v4.9] On Tue, Jan 17, 2017 at 2:34 PM, Amir Goldstein <amir73il@gmail.com> 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@vger.kernel.org> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > 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 > ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ovl: fix possible use after free on redirect dir lookup 2017-01-17 20:06 ` Amir Goldstein @ 2017-01-18 4:46 ` Amir Goldstein 0 siblings, 0 replies; 7+ messages in thread From: Amir Goldstein @ 2017-01-18 4:46 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-unionfs, stable [v4.9] On Tue, Jan 17, 2017 at 10:06 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Tue, Jan 17, 2017 at 2:34 PM, Amir Goldstein <amir73il@gmail.com> 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@vger.kernel.org> And it's not 4.9 stable its 4.10 fixes of course @stable, sorry about this noise >> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> --- >> >> 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 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] ovl: fix possible use after free on redirect dir lookup 2017-01-17 12:34 [PATCH] ovl: fix possible use after free on redirect dir lookup Amir Goldstein 2017-01-17 20:06 ` Amir Goldstein @ 2017-01-18 8:15 ` Amir Goldstein 2017-01-18 10:38 ` Miklos Szeredi 1 sibling, 1 reply; 7+ messages in thread From: Amir Goldstein @ 2017-01-18 8:15 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-unionfs 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. Fixes: 02b69b284cd7 ("ovl: lookup redirects") Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/overlayfs/namei.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) Miklos, I was pretty close with the test case yesterday, but got it right this morning: https://github.com/amir73il/unionmount-testsuite/commit/fac638f1d1d0191ca3590f22519a4715a96d5bf2 './run --ov=20 rename-move-dir' fails on kernel 4.10-rc1: ... TEST rename-move-dir.py:123: Rename populated dir and parent and move into another ./run --rename /mnt/a/dir106/pop /mnt/a/dir106/popx #no_recycle ./run --rename /mnt/a/dir106 /mnt/a/dir106x #recycle ./run --rename /mnt/a/dir106x/popx /mnt/a/empty106/popx #recycle ... ./run --open-file /mnt/a/empty106/popx -r -d /mnt/a/empty106/popx/b: File is missing I actually wrote the right test description yesterday: ("Rename populated dir and parent and move self into another dir") but implemented instead: ("Rename populated dir and child and move self into another dir") Both cases get to a point of iterating over freed memory and that causes iteration to stop (*s != '/') on my debug kernel. Latter case would have stopped iterating anyway (postlen == 0), while Former case would have continued iterating, but does not, so the test case fails to lookup inside the moved directory. Amir. v2: - Remove stable tag - Add Fixes tag v1: - Initial fix 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ovl: fix possible use after free on redirect dir lookup 2017-01-18 8:15 ` [PATCH v2] " Amir Goldstein @ 2017-01-18 10:38 ` Miklos Szeredi 2017-01-18 11:40 ` Amir Goldstein 0 siblings, 1 reply; 7+ messages in thread From: Miklos Szeredi @ 2017-01-18 10:38 UTC (permalink / raw) To: Amir Goldstein; +Cc: linux-unionfs Amir, Thanks for spotting this bug and for the patch. I simplified it a bit (appended) 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. Only compile tested. Please let me know if this works or not. Thanks, Miklos --- From: Amir Goldstein <amir73il@gmail.com> 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@gmail.com> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- 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); + 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ovl: fix possible use after free on redirect dir lookup 2017-01-18 10:38 ` Miklos Szeredi @ 2017-01-18 11:40 ` Amir Goldstein 2017-01-18 14:55 ` Miklos Szeredi 0 siblings, 1 reply; 7+ messages in thread From: Amir Goldstein @ 2017-01-18 11:40 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-unionfs On Wed, Jan 18, 2017 at 12:38 PM, Miklos Szeredi <miklos@szeredi.hu> 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@gmail.com> > 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@gmail.com> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] ovl: fix possible use after free on redirect dir lookup 2017-01-18 11:40 ` Amir Goldstein @ 2017-01-18 14:55 ` Miklos Szeredi 0 siblings, 0 replies; 7+ messages in thread From: Miklos Szeredi @ 2017-01-18 14:55 UTC (permalink / raw) To: Amir Goldstein; +Cc: linux-unionfs@vger.kernel.org On Wed, Jan 18, 2017 at 12:40 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Wed, Jan 18, 2017 at 12:38 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> Only compile tested. Please let me know if this works or not. >> > > It works. Asserts added and pushed. Thanks, Miklos ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-01-18 15:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-17 12:34 [PATCH] ovl: fix possible use after free on redirect dir lookup Amir Goldstein 2017-01-17 20:06 ` Amir Goldstein 2017-01-18 4:46 ` Amir Goldstein 2017-01-18 8:15 ` [PATCH v2] " Amir Goldstein 2017-01-18 10:38 ` Miklos Szeredi 2017-01-18 11:40 ` Amir Goldstein 2017-01-18 14:55 ` Miklos Szeredi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox