The existing logic uses strnlen_user() to calculate the length of the
memfd name from userspace and then copies the string into a buffer using
copy_from_user(). This is error-prone, as the string length
could have changed between the time when it was calculated and when the
string was copied. The existing logic handles this by ensuring that the
last byte in the buffer is the terminating zero.

This handling is contrived and can better be handled by using
strncpy_from_user(), which gets the length of the string and copies
it in one shot. Therefore, simplify the logic for copying the memfd
name by using strncpy_from_user().

No functional change.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
---
 mm/memfd.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/mm/memfd.c b/mm/memfd.c
index bf0c2d97b940..5b7c5892ba64 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -396,26 +396,18 @@ static char *alloc_name(const char __user *uname)
 	char *name;
 	long len;
 
-	/* length includes terminating zero */
-	len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
-	if (len <= 0)
-		return ERR_PTR(-EFAULT);
-	if (len > MFD_NAME_MAX_LEN + 1)
-		return ERR_PTR(-EINVAL);
-
-	name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
+	name = kmalloc(NAME_MAX + 1, GFP_KERNEL);
 	if (!name)
 		return ERR_PTR(-ENOMEM);
 
 	strcpy(name, MFD_NAME_PREFIX);
-	if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
+	/* returned length does not include terminating zero */
+	len = strncpy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, MFD_NAME_MAX_LEN + 1);
+	if (len < 0) {
 		error = -EFAULT;
 		goto err_name;
-	}
-
-	/* terminating-zero may have changed after strnlen_user() returned */
-	if (name[len + MFD_NAME_PREFIX_LEN - 1]) {
-		error = -EFAULT;
+	} else if (len > MFD_NAME_MAX_LEN) {
+		error = -EINVAL;
 		goto err_name;
 	}
 
-- 
2.47.1.613.gc27f4b7a9f-goog

On Thu, Jan 09, 2025 at 10:59:05AM -0800, Isaac J. Manjarres wrote:
> The existing logic uses strnlen_user() to calculate the length of the
> memfd name from userspace and then copies the string into a buffer using
> copy_from_user(). This is error-prone, as the string length
> could have changed between the time when it was calculated and when the
> string was copied. The existing logic handles this by ensuring that the
> last byte in the buffer is the terminating zero.
>
> This handling is contrived and can better be handled by using
> strncpy_from_user(), which gets the length of the string and copies
> it in one shot. Therefore, simplify the logic for copying the memfd
> name by using strncpy_from_user().
>
> No functional change.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>

LGTM,

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  mm/memfd.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/mm/memfd.c b/mm/memfd.c
> index bf0c2d97b940..5b7c5892ba64 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -396,26 +396,18 @@ static char *alloc_name(const char __user *uname)
>  	char *name;
>  	long len;
>
> -	/* length includes terminating zero */
> -	len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
> -	if (len <= 0)
> -		return ERR_PTR(-EFAULT);
> -	if (len > MFD_NAME_MAX_LEN + 1)
> -		return ERR_PTR(-EINVAL);
> -
> -	name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
> +	name = kmalloc(NAME_MAX + 1, GFP_KERNEL);
>  	if (!name)
>  		return ERR_PTR(-ENOMEM);
>
>  	strcpy(name, MFD_NAME_PREFIX);
> -	if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
> +	/* returned length does not include terminating zero */
> +	len = strncpy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, MFD_NAME_MAX_LEN + 1);
> +	if (len < 0) {
>  		error = -EFAULT;
>  		goto err_name;
> -	}
> -
> -	/* terminating-zero may have changed after strnlen_user() returned */
> -	if (name[len + MFD_NAME_PREFIX_LEN - 1]) {
> -		error = -EFAULT;
> +	} else if (len > MFD_NAME_MAX_LEN) {
> +		error = -EINVAL;
>  		goto err_name;
>  	}
>
> --
> 2.47.1.613.gc27f4b7a9f-goog
>

On Fri, Jan 10, 2025 at 12:48:44PM +0000, Lorenzo Stoakes wrote:
> On Thu, Jan 09, 2025 at 10:59:05AM -0800, Isaac J. Manjarres wrote:
> > The existing logic uses strnlen_user() to calculate the length of the
> > memfd name from userspace and then copies the string into a buffer using
> > copy_from_user(). This is error-prone, as the string length
> > could have changed between the time when it was calculated and when the
> > string was copied. The existing logic handles this by ensuring that the
> > last byte in the buffer is the terminating zero.
> >
> > This handling is contrived and can better be handled by using
> > strncpy_from_user(), which gets the length of the string and copies
> > it in one shot. Therefore, simplify the logic for copying the memfd
> > name by using strncpy_from_user().
> >
> > No functional change.
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
> 
> LGTM,
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> 

Thank you!

--Isaac