| Message ID | 20240604155257.109500-4-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | whack user_path_at_empty, cleanup getname_flags | expand |
On Tue, Jun 04, 2024 at 05:52:57PM +0200, Mateusz Guzik wrote:
> Check for an error while copying and no path in one branch.
Fine to move that check up but we need to redo it after we recopied.
It's extremely unlikely but the contents could've changed iirc. I can
fix that up though.
On Wed, Jun 5, 2024 at 5:03 PM Christian Brauner <brauner@kernel.org> wrote: > > On Tue, Jun 04, 2024 at 05:52:57PM +0200, Mateusz Guzik wrote: > > Check for an error while copying and no path in one branch. > > Fine to move that check up but we need to redo it after we recopied. > It's extremely unlikely but the contents could've changed iirc. I can > fix that up though. Oh I see, you mean the buffer might have initially been too big so it landed in the zmalloc fallback, but by that time userspace might have played games and slapped a NUL char in there. Fair, but also trivial to fix up, so I would appreciate if you sorted it out, especially since you offered :)
On Tue 04-06-24 17:52:57, Mateusz Guzik wrote: > Check for an error while copying and no path in one branch. > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> Looks good to me modulo the need for rechecking Christian already pointed out. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/namei.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/fs/namei.c b/fs/namei.c > index 950ad6bdd9fe..f25dcb9077dd 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -148,9 +148,20 @@ getname_flags(const char __user *filename, int flags) > result->name = kname; > > len = strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX); > - if (unlikely(len < 0)) { > - __putname(result); > - return ERR_PTR(len); > + /* > + * Handle both empty path and copy failure in one go. > + */ > + if (unlikely(len <= 0)) { > + if (unlikely(len < 0)) { > + __putname(result); > + return ERR_PTR(len); > + } > + > + /* The empty path is special. */ > + if (!(flags & LOOKUP_EMPTY)) { > + __putname(result); > + return ERR_PTR(-ENOENT); > + } > } > > /* > @@ -188,14 +199,6 @@ getname_flags(const char __user *filename, int flags) > } > > atomic_set(&result->refcnt, 1); > - /* The empty path is special. */ > - if (unlikely(!len)) { > - if (!(flags & LOOKUP_EMPTY)) { > - putname(result); > - return ERR_PTR(-ENOENT); > - } > - } > - > result->uptr = filename; > result->aname = NULL; > audit_getname(result); > -- > 2.39.2 >
diff --git a/fs/namei.c b/fs/namei.c index 950ad6bdd9fe..f25dcb9077dd 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -148,9 +148,20 @@ getname_flags(const char __user *filename, int flags) result->name = kname; len = strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX); - if (unlikely(len < 0)) { - __putname(result); - return ERR_PTR(len); + /* + * Handle both empty path and copy failure in one go. + */ + if (unlikely(len <= 0)) { + if (unlikely(len < 0)) { + __putname(result); + return ERR_PTR(len); + } + + /* The empty path is special. */ + if (!(flags & LOOKUP_EMPTY)) { + __putname(result); + return ERR_PTR(-ENOENT); + } } /* @@ -188,14 +199,6 @@ getname_flags(const char __user *filename, int flags) } atomic_set(&result->refcnt, 1); - /* The empty path is special. */ - if (unlikely(!len)) { - if (!(flags & LOOKUP_EMPTY)) { - putname(result); - return ERR_PTR(-ENOENT); - } - } - result->uptr = filename; result->aname = NULL; audit_getname(result);
Check for an error while copying and no path in one branch. Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- fs/namei.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)