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(-)

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

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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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 :)

-- 
Mateusz Guzik <mjguzik gmail.com>