[PATCH v2 0/2] Cleanup for memfd_create()

[PATCH v2 0/2] Cleanup for memfd_create()

[Isaac J. Manjarres]

memfd_create() handles all of its logic in a single function. Some of
the logic in the function is also somewhat contrived (i.e. copying the
memfd name from userpace).

This series aims to cleanup memfd_create() by splitting out the logic
into helper functions, and simplifying the memfd name copying to make
the code easier to follow.

This has no intended functional changes.

Changes from v1 ==> v2:

- Rebased on top of the mm-unstable branch instead of Linus' master
  branch. Base commit on mm-unstable: ca95745c20ad ("mm/memmap: prevent
  double scanning of memmap by kmemleak").

Links:

v1: https://lore.kernel.org/all/20250102230658.1112261-1-isaacmanjarres@google.com/#t

Isaac J. Manjarres (2):
  mm/memfd: Refactor and cleanup the logic in memfd_create()
  mm/memfd: Use strncpy_from_user() to read memfd name

 mm/memfd.c | 101 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 59 insertions(+), 42 deletions(-)

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v2 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()

[Isaac J. Manjarres]

memfd_create() is a pretty busy function that could be easier to read
if some of the logic was split out into helper functions.

Therefore, split the flags check, name creation, and file creation into
their own helper functions, and create the file structure before
creating the memfd. This allows for simplifying the error handling path
in memfd_create().

No functional change.

Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
---
 mm/memfd.c | 87 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 56 insertions(+), 31 deletions(-)

diff --git a/mm/memfd.c b/mm/memfd.c
index 5f5a23c9051d..a9430090bb20 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -369,16 +369,8 @@ int memfd_check_seals_mmap(struct file *file, unsigned long *vm_flags_ptr)
 	return err;
 }
 
-SYSCALL_DEFINE2(memfd_create,
-		const char __user *, uname,
-		unsigned int, flags)
+static int memfd_validate_flags(unsigned int flags)
 {
-	unsigned int *file_seals;
-	struct file *file;
-	int fd, error;
-	char *name;
-	long len;
-
 	if (!(flags & MFD_HUGETLB)) {
 		if (flags & ~(unsigned int)MFD_ALL_FLAGS)
 			return -EINVAL;
@@ -393,20 +385,25 @@ SYSCALL_DEFINE2(memfd_create,
 	if ((flags & MFD_EXEC) && (flags & MFD_NOEXEC_SEAL))
 		return -EINVAL;
 
-	error = check_sysctl_memfd_noexec(&flags);
-	if (error < 0)
-		return error;
+	return check_sysctl_memfd_noexec(&flags);
+}
+
+static char *memfd_create_name(const char __user *uname)
+{
+	int error;
+	char *name;
+	long len;
 
 	/* length includes terminating zero */
 	len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
 	if (len <= 0)
-		return -EFAULT;
+		return ERR_PTR(-EFAULT);
 	if (len > MFD_NAME_MAX_LEN + 1)
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 
 	name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
 	if (!name)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	strcpy(name, MFD_NAME_PREFIX);
 	if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
@@ -420,11 +417,22 @@ SYSCALL_DEFINE2(memfd_create,
 		goto err_name;
 	}
 
-	fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
-	if (fd < 0) {
-		error = fd;
-		goto err_name;
-	}
+	return name;
+
+err_name:
+	kfree(name);
+	return ERR_PTR(error);
+}
+
+static struct file *memfd_file_create(const char *name, unsigned int flags)
+{
+	unsigned int *file_seals;
+	struct file *file;
+	int error;
+
+	error = memfd_validate_flags(flags);
+	if (error < 0)
+		return ERR_PTR(error);
 
 	if (flags & MFD_HUGETLB) {
 		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
@@ -433,10 +441,8 @@ SYSCALL_DEFINE2(memfd_create,
 					MFD_HUGE_MASK);
 	} else
 		file = shmem_file_setup(name, 0, VM_NORESERVE);
-	if (IS_ERR(file)) {
-		error = PTR_ERR(file);
-		goto err_fd;
-	}
+	if (IS_ERR(file))
+		return file;
 	file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
 	file->f_flags |= O_LARGEFILE;
 
@@ -456,13 +462,32 @@ SYSCALL_DEFINE2(memfd_create,
 			*file_seals &= ~F_SEAL_SEAL;
 	}
 
-	fd_install(fd, file);
-	kfree(name);
-	return fd;
+	return file;
+}
 
-err_fd:
-	put_unused_fd(fd);
-err_name:
+SYSCALL_DEFINE2(memfd_create,
+		const char __user *, uname,
+		unsigned int, flags)
+{
+	struct file *file;
+	int fd;
+	char *name;
+
+	name = memfd_create_name(uname);
+	if (IS_ERR(name))
+		return PTR_ERR(name);
+
+	file = memfd_file_create(name, flags);
+	/* name is not needed beyond this point. */
 	kfree(name);
-	return error;
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+
+	fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
+	if (fd >= 0)
+		fd_install(fd, file);
+	else
+		fput(file);
+
+	return fd;
 }
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()

[Alice Ryhl]

On Tue, Jan 7, 2025 at 7:48 PM Isaac J. Manjarres
<isaacmanjarres@google.com> wrote:
> +SYSCALL_DEFINE2(memfd_create,
> +               const char __user *, uname,
> +               unsigned int, flags)
> +{
> +       struct file *file;
> +       int fd;
> +       char *name;
> +
> +       name = memfd_create_name(uname);
> +       if (IS_ERR(name))
> +               return PTR_ERR(name);
> +
> +       file = memfd_file_create(name, flags);
> +       /* name is not needed beyond this point. */
>         kfree(name);
> -       return error;
> +       if (IS_ERR(file))
> +               return PTR_ERR(file);
> +
> +       fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
> +       if (fd >= 0)
> +               fd_install(fd, file);
> +       else
> +               fput(file);

You changed the order so that get_unused_fd_flags() happens after
creating the file, so the error path now does fput(file) instead of
put_unused_fd(fd). Is there a reason for this? I would generally
assume that calling get_unused_fd_flags() first is better.

Otherwise this LGTM.


Alice

Re: [PATCH v2 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()

[Isaac Manjarres]

On Wed, Jan 08, 2025 at 02:31:58PM +0100, Alice Ryhl wrote:
> On Tue, Jan 7, 2025 at 7:48 PM Isaac J. Manjarres
> <isaacmanjarres@google.com> wrote:
> > +SYSCALL_DEFINE2(memfd_create,
> > +               const char __user *, uname,
> > +               unsigned int, flags)
> > +{
> > +       struct file *file;
> > +       int fd;
> > +       char *name;
> > +
> > +       name = memfd_create_name(uname);
> > +       if (IS_ERR(name))
> > +               return PTR_ERR(name);
> > +
> > +       file = memfd_file_create(name, flags);
> > +       /* name is not needed beyond this point. */
> >         kfree(name);
> > -       return error;
> > +       if (IS_ERR(file))
> > +               return PTR_ERR(file);
> > +
> > +       fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
> > +       if (fd >= 0)
> > +               fd_install(fd, file);
> > +       else
> > +               fput(file);
> 
> You changed the order so that get_unused_fd_flags() happens after
> creating the file, so the error path now does fput(file) instead of
> put_unused_fd(fd). Is there a reason for this? I would generally
> assume that calling get_unused_fd_flags() first is better.

Thanks for taking a look at this, Alice!

I changed the order so that the code had a more logical structure
where we create objects and use them right away, as opposed to getting
an fd, then creating the file to associate with that file descriptor,
and then actually associating it.

I also structured the code to get rid of the gotos in this function to
make it easier to follow. It also made sense to me to fold the flags
validation into memfd_file_create() since that's where the flags are
used the most anyway, and it also makes sense to validate the flags
first, so reordering the file creation and fd creation allowed me to
do that.

I'm open to restoring the order back to how it was though. Is there a
reason for why get_unused_fd_flags() first is better?

Thanks,
Isaac

Re: [PATCH v2 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()

[Lorenzo Stoakes]

On Tue, Jan 07, 2025 at 10:48:01AM -0800, Isaac J. Manjarres wrote:
> memfd_create() is a pretty busy function that could be easier to read
> if some of the logic was split out into helper functions.
>
> Therefore, split the flags check, name creation, and file creation into
> their own helper functions, and create the file structure before
> creating the memfd. This allows for simplifying the error handling path
> in memfd_create().

I do like the intent of this change, but I think this needs some tweaking.

I wish the diff algorithm would do a little better here, because it's quite
hard to follow. In no way your fault that :) difftastic hopefully can help
me here...

>
> No functional change.
>
> Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
> ---
>  mm/memfd.c | 87 +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 56 insertions(+), 31 deletions(-)
>
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 5f5a23c9051d..a9430090bb20 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -369,16 +369,8 @@ int memfd_check_seals_mmap(struct file *file, unsigned long *vm_flags_ptr)
>  	return err;
>  }
>
> -SYSCALL_DEFINE2(memfd_create,
> -		const char __user *, uname,
> -		unsigned int, flags)
> +static int memfd_validate_flags(unsigned int flags)

For static functions the memfd_ prefix is redundant, please strip them. We
know we're in mm/memfd.c which is context enough for these internal
helpers!

>  {
> -	unsigned int *file_seals;
> -	struct file *file;
> -	int fd, error;
> -	char *name;
> -	long len;
> -
>  	if (!(flags & MFD_HUGETLB)) {
>  		if (flags & ~(unsigned int)MFD_ALL_FLAGS)
>  			return -EINVAL;
> @@ -393,20 +385,25 @@ SYSCALL_DEFINE2(memfd_create,
>  	if ((flags & MFD_EXEC) && (flags & MFD_NOEXEC_SEAL))
>  		return -EINVAL;
>
> -	error = check_sysctl_memfd_noexec(&flags);
> -	if (error < 0)
> -		return error;
> +	return check_sysctl_memfd_noexec(&flags);

More importantly - this is broken...

The check_sysctl_memfd_noexec() function is _changing_ flags, which you now
discard.

This also renders 'validate' in the name a little inaccurate (hey naming is
hard :), perhaps 'sanitise_flags()'?

Anyway you should pass flags as a pointer (even if that's yuck) and rename.

> +}
> +
> +static char *memfd_create_name(const char __user *uname)

Again, strip memfd_ prefix please.

Also I don't know what 'create' means here. Given the function you're
interacting with is memfd_create() it's rendered a little vague.

I'd say 'alloc_name()' would be better.

> +{
> +	int error;
> +	char *name;
> +	long len;
>
>  	/* length includes terminating zero */
>  	len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
>  	if (len <= 0)
> -		return -EFAULT;
> +		return ERR_PTR(-EFAULT);

Not sure if this is necessary, I mean I guess technically... but feels like
it's adding a bunch of noise.

I know you refactor this whole thing in the next commit so maybe to reduce
the size of this commit you could drop these changes here and keep the bare
minimum before you change the function again?


>  	if (len > MFD_NAME_MAX_LEN + 1)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>
>  	name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
>  	if (!name)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>
>  	strcpy(name, MFD_NAME_PREFIX);
>  	if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
> @@ -420,11 +417,22 @@ SYSCALL_DEFINE2(memfd_create,
>  		goto err_name;
>  	}

>
> -	fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
> -	if (fd < 0) {
> -		error = fd;
> -		goto err_name;
> -	}
> +	return name;
> +
> +err_name:
> +	kfree(name);
> +	return ERR_PTR(error);
> +}
> +
> +static struct file *memfd_file_create(const char *name, unsigned int flags)

I really am not a great fan of this name, memfd_ prefix obviously has to
go, but 'file_create' when the actual system call is 'memfd_create".

Again, naming eh? It is hard :)

alloc_file() probably works best as you are in fact allocating memory for
this.

Then, as mentioned below, restore the original ordering of fd assignment in
the syscall, do so before file allocation, and install the file into the fd
afterwards.

> +{
> +	unsigned int *file_seals;
> +	struct file *file;
> +	int error;
> +
> +	error = memfd_validate_flags(flags);
> +	if (error < 0)
> +		return ERR_PTR(error);

I'm not actually sure why you put this here, it seems quite
arbitrary. Let's invoke this from the syscall so we neatly divide the logic
into each part rather than dividing into different parts one of which is
invoked by another.

And obviously make sure the original ordering is restored.

>
>  	if (flags & MFD_HUGETLB) {
>  		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
> @@ -433,10 +441,8 @@ SYSCALL_DEFINE2(memfd_create,
>  					MFD_HUGE_MASK);
>  	} else
>  		file = shmem_file_setup(name, 0, VM_NORESERVE);

I know not to do with you, and strictly this is probably in line with
kernel code style, but this dangling else _kills_ me.

Could we put { } around it? Risking invoking the ire of the strict
adherents of the coding style perhaps here :P

> -	if (IS_ERR(file)) {
> -		error = PTR_ERR(file);
> -		goto err_fd;
> -	}
> +	if (IS_ERR(file))
> +		return file;
>  	file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
>  	file->f_flags |= O_LARGEFILE;
>
> @@ -456,13 +462,32 @@ SYSCALL_DEFINE2(memfd_create,
>  			*file_seals &= ~F_SEAL_SEAL;
>  	}
>
> -	fd_install(fd, file);
> -	kfree(name);
> -	return fd;
> +	return file;
> +}
>
> -err_fd:
> -	put_unused_fd(fd);
> -err_name:
> +SYSCALL_DEFINE2(memfd_create,
> +		const char __user *, uname,
> +		unsigned int, flags)
> +{
> +	struct file *file;
> +	int fd;
> +	char *name;
> +
> +	name = memfd_create_name(uname);
> +	if (IS_ERR(name))
> +		return PTR_ERR(name);

You're changing the ordering of checks which is user-visible. Previously
the flags would be validated first, now you're 'creating' the name (not
sure this is a great name - naming is hard obviously but this doesn't
really tell me what you intend here, I'll highlight in that bit of code I
guess).

Please try to keep the order of checks the same and validate the flags first.

> +
> +	file = memfd_file_create(name, flags);
> +	/* name is not needed beyond this point. */

This is nice to highlight! Though it absolutely SUCKS to kmalloc() some
memory then instantly discard it like this. But I suppose we have no choice
here.

>  	kfree(name);
> -	return error;
> +	if (IS_ERR(file))
> +		return PTR_ERR(file);
> +
> +	fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
> +	if (fd >= 0)
> +		fd_install(fd, file);
> +	else
> +		fput(file);

You've changed the ordering of this again, and you're not doing anything to
free file if something goes wrong with fd allocation? That's a leak no?

Please reinstate the original ordering.

It's strange to open code this when we don't open code other things... but
perhaps for a few lines it's ok.

> +
> +	return fd;
>  }
> --
> 2.47.1.613.gc27f4b7a9f-goog
>

Re: [PATCH v2 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()

[Isaac Manjarres]

On Wed, Jan 08, 2025 at 06:30:54PM +0000, Lorenzo Stoakes wrote:
> On Tue, Jan 07, 2025 at 10:48:01AM -0800, Isaac J. Manjarres wrote:
> > memfd_create() is a pretty busy function that could be easier to read
> > if some of the logic was split out into helper functions.
> >
> > Therefore, split the flags check, name creation, and file creation into
> > their own helper functions, and create the file structure before
> > creating the memfd. This allows for simplifying the error handling path
> > in memfd_create().
> 
> I do like the intent of this change, but I think this needs some tweaking.
> 
> I wish the diff algorithm would do a little better here, because it's quite
> hard to follow. In no way your fault that :) difftastic hopefully can help
> me here...
> 
> >
> > No functional change.
> >
> > Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
> > ---
> >  mm/memfd.c | 87 +++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 56 insertions(+), 31 deletions(-)
> >
> > diff --git a/mm/memfd.c b/mm/memfd.c
> > index 5f5a23c9051d..a9430090bb20 100644
> > --- a/mm/memfd.c
> > +++ b/mm/memfd.c
> > @@ -369,16 +369,8 @@ int memfd_check_seals_mmap(struct file *file, unsigned long *vm_flags_ptr)
> >  	return err;
> >  }
> >
> > -SYSCALL_DEFINE2(memfd_create,
> > -		const char __user *, uname,
> > -		unsigned int, flags)
> > +static int memfd_validate_flags(unsigned int flags)
> 
> For static functions the memfd_ prefix is redundant, please strip them. We
> know we're in mm/memfd.c which is context enough for these internal
> helpers!
> 

Thanks for taking a look at this patch! Understood, I'll go ahead and
strip those prefixes.

> >  {
> > -	unsigned int *file_seals;
> > -	struct file *file;
> > -	int fd, error;
> > -	char *name;
> > -	long len;
> > -
> >  	if (!(flags & MFD_HUGETLB)) {
> >  		if (flags & ~(unsigned int)MFD_ALL_FLAGS)
> >  			return -EINVAL;
> > @@ -393,20 +385,25 @@ SYSCALL_DEFINE2(memfd_create,
> >  	if ((flags & MFD_EXEC) && (flags & MFD_NOEXEC_SEAL))
> >  		return -EINVAL;
> >
> > -	error = check_sysctl_memfd_noexec(&flags);
> > -	if (error < 0)
> > -		return error;
> > +	return check_sysctl_memfd_noexec(&flags);
> 
> More importantly - this is broken...
> 
> The check_sysctl_memfd_noexec() function is _changing_ flags, which you now
> discard.
> 
> This also renders 'validate' in the name a little inaccurate (hey naming is
> hard :), perhaps 'sanitise_flags()'?
> 
> Anyway you should pass flags as a pointer (even if that's yuck) and rename.
> 

Thanks for catching that. I will go ahead and use a pointer to the flags
and rename the function as well.

> > +}
> > +
> > +static char *memfd_create_name(const char __user *uname)
> 
> Again, strip memfd_ prefix please.
> 
> Also I don't know what 'create' means here. Given the function you're
> interacting with is memfd_create() it's rendered a little vague.
> 
> I'd say 'alloc_name()' would be better.

Acknowledged, I will go ahead and do that in v3 of the patch.

> > +{
> > +	int error;
> > +	char *name;
> > +	long len;
> >
> >  	/* length includes terminating zero */
> >  	len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
> >  	if (len <= 0)
> > -		return -EFAULT;
> > +		return ERR_PTR(-EFAULT);
> 
> Not sure if this is necessary, I mean I guess technically... but feels like
> it's adding a bunch of noise.
> 
> I know you refactor this whole thing in the next commit so maybe to reduce
> the size of this commit you could drop these changes here and keep the bare
> minimum before you change the function again?
> 

To make sure I understood correctly, do you mean that I should combine
introducing alloc_name() and changing the handling for how the name is
allocated in the next patch instead and just leave this logic alone
in this patch?

> >  	if (len > MFD_NAME_MAX_LEN + 1)
> > -		return -EINVAL;
> > +		return ERR_PTR(-EINVAL);
> >
> >  	name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
> >  	if (!name)
> > -		return -ENOMEM;
> > +		return ERR_PTR(-ENOMEM);
> >
> >  	strcpy(name, MFD_NAME_PREFIX);
> >  	if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
> > @@ -420,11 +417,22 @@ SYSCALL_DEFINE2(memfd_create,
> >  		goto err_name;
> >  	}
> 
> >
> > -	fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
> > -	if (fd < 0) {
> > -		error = fd;
> > -		goto err_name;
> > -	}
> > +	return name;
> > +
> > +err_name:
> > +	kfree(name);
> > +	return ERR_PTR(error);
> > +}
> > +
> > +static struct file *memfd_file_create(const char *name, unsigned int flags)
> 
> I really am not a great fan of this name, memfd_ prefix obviously has to
> go, but 'file_create' when the actual system call is 'memfd_create".
> 
> Again, naming eh? It is hard :)
> 
> alloc_file() probably works best as you are in fact allocating memory for
> this.

Yes, that makes sense to me.

> 
> Then, as mentioned below, restore the original ordering of fd assignment in
> the syscall, do so before file allocation, and install the file into the fd
> afterwards.
> 
> > +{
> > +	unsigned int *file_seals;
> > +	struct file *file;
> > +	int error;
> > +
> > +	error = memfd_validate_flags(flags);
> > +	if (error < 0)
> > +		return ERR_PTR(error);
> 
> I'm not actually sure why you put this here, it seems quite
> arbitrary. Let's invoke this from the syscall so we neatly divide the logic
> into each part rather than dividing into different parts one of which is
> invoked by another.

I put the flags validation here since the flags are mostly used in this
function, so it made sense to me embed the flags validation into here.
However, I do understand how splitting this out as it was before makes
more sense now, especially since the flags can change. It seems odd
for alloc_file() to have a side effect of changing the flags, so I
will place the flags sanitization back to where it was.

> 
> And obviously make sure the original ordering is restored.
> 
> >
> >  	if (flags & MFD_HUGETLB) {
> >  		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
> > @@ -433,10 +441,8 @@ SYSCALL_DEFINE2(memfd_create,
> >  					MFD_HUGE_MASK);
> >  	} else
> >  		file = shmem_file_setup(name, 0, VM_NORESERVE);
> 
> I know not to do with you, and strictly this is probably in line with
> kernel code style, but this dangling else _kills_ me.
> 
> Could we put { } around it? Risking invoking the ire of the strict
> adherents of the coding style perhaps here :P
> 

Absolutely!

> > -	if (IS_ERR(file)) {
> > -		error = PTR_ERR(file);
> > -		goto err_fd;
> > -	}
> > +	if (IS_ERR(file))
> > +		return file;
> >  	file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
> >  	file->f_flags |= O_LARGEFILE;
> >
> > @@ -456,13 +462,32 @@ SYSCALL_DEFINE2(memfd_create,
> >  			*file_seals &= ~F_SEAL_SEAL;
> >  	}
> >
> > -	fd_install(fd, file);
> > -	kfree(name);
> > -	return fd;
> > +	return file;
> > +}
> >
> > -err_fd:
> > -	put_unused_fd(fd);
> > -err_name:
> > +SYSCALL_DEFINE2(memfd_create,
> > +		const char __user *, uname,
> > +		unsigned int, flags)
> > +{
> > +	struct file *file;
> > +	int fd;
> > +	char *name;
> > +
> > +	name = memfd_create_name(uname);
> > +	if (IS_ERR(name))
> > +		return PTR_ERR(name);
> 
> You're changing the ordering of checks which is user-visible. Previously
> the flags would be validated first, now you're 'creating' the name (not
> sure this is a great name - naming is hard obviously but this doesn't
> really tell me what you intend here, I'll highlight in that bit of code I
> guess).
> 
> Please try to keep the order of checks the same and validate the flags first.
> 

Will do.

> > +
> > +	file = memfd_file_create(name, flags);
> > +	/* name is not needed beyond this point. */
> 
> This is nice to highlight! Though it absolutely SUCKS to kmalloc() some
> memory then instantly discard it like this. But I suppose we have no choice
> here.
> 

What if instead, we allocate a buffer on the stack and change
alloc_name() to populate_name() and have it take in a pointer
to that buffer and join the memfd name prefix and copy the
name from userspace into that buffer? That avoids having to
dynamically allocate a buffer that gets freed right away,
and also gets rid of the cleanup, since that memory will be
deallocated when we return from the function.

> >  	kfree(name);
> > -	return error;
> > +	if (IS_ERR(file))
> > +		return PTR_ERR(file);
> > +
> > +	fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
> > +	if (fd >= 0)
> > +		fd_install(fd, file);
> > +	else
> > +		fput(file);
> 
> You've changed the ordering of this again, and you're not doing anything to
> free file if something goes wrong with fd allocation? That's a leak no?
> 

file only has one reference to it at this point, and in the else branch,
or failure case, fput() is invoked to drop the reference on file, which
will cause it to be freed, so I don't think it's a leak, unless I missed
something?

> Please reinstate the original ordering.

I'm happy to reinstate the original ordering, but for my knowledge, is
there a reason as to why fd allocation before file structure allocation
is preferred instead of the other way around?

Thanks,
Isaac

> It's strange to open code this when we don't open code other things... but
> perhaps for a few lines it's ok.
> 
> > +
> > +	return fd;
> >  }
> > --
> > 2.47.1.613.gc27f4b7a9f-goog
> >

Re: [PATCH v2 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()

[Lorenzo Stoakes]

On Wed, Jan 08, 2025 at 12:04:40PM -0800, Isaac Manjarres wrote:

[ snipping stuff for brevity - thanks for taking a look at the other raised
  issues! ]

> > > +{
> > > +	int error;
> > > +	char *name;
> > > +	long len;
> > >
> > >  	/* length includes terminating zero */
> > >  	len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
> > >  	if (len <= 0)
> > > -		return -EFAULT;
> > > +		return ERR_PTR(-EFAULT);
> >
> > Not sure if this is necessary, I mean I guess technically... but feels like
> > it's adding a bunch of noise.
> >
> > I know you refactor this whole thing in the next commit so maybe to reduce
> > the size of this commit you could drop these changes here and keep the bare
> > minimum before you change the function again?
> >
>
> To make sure I understood correctly, do you mean that I should combine
> introducing alloc_name() and changing the handling for how the name is
> allocated in the next patch instead and just leave this logic alone
> in this patch?

It's not actually a huge big deal, I mean just drop the ERR_PTR() stuff as
you refactor again just after.

But this is optional!


> >
> > Then, as mentioned below, restore the original ordering of fd assignment in
> > the syscall, do so before file allocation, and install the file into the fd
> > afterwards.
> >
> > > +{
> > > +	unsigned int *file_seals;
> > > +	struct file *file;
> > > +	int error;
> > > +
> > > +	error = memfd_validate_flags(flags);
> > > +	if (error < 0)
> > > +		return ERR_PTR(error);
> >
> > I'm not actually sure why you put this here, it seems quite
> > arbitrary. Let's invoke this from the syscall so we neatly divide the logic
> > into each part rather than dividing into different parts one of which is
> > invoked by another.
>
> I put the flags validation here since the flags are mostly used in this
> function, so it made sense to me embed the flags validation into here.
> However, I do understand how splitting this out as it was before makes
> more sense now, especially since the flags can change. It seems odd
> for alloc_file() to have a side effect of changing the flags, so I
> will place the flags sanitization back to where it was.

Ack sure, I mean the original was horrid, so I get it.

But to me makes more sense to divide up the tasks into smaller functions
(again, I love the intent of this series, this is a very Lorenzo-style
thing to do so very en-vogue-Stoakes which I naturally appreciate :) - and
this is just another part that is a little simpler to separate out at the
top level.

It's not a huge thing but I think improves the code.

>
> >
> > And obviously make sure the original ordering is restored.
> >
> > >
> > >  	if (flags & MFD_HUGETLB) {
> > >  		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
> > > @@ -433,10 +441,8 @@ SYSCALL_DEFINE2(memfd_create,
> > >  					MFD_HUGE_MASK);
> > >  	} else
> > >  		file = shmem_file_setup(name, 0, VM_NORESERVE);
> >
> > I know not to do with you, and strictly this is probably in line with
> > kernel code style, but this dangling else _kills_ me.
> >
> > Could we put { } around it? Risking invoking the ire of the strict
> > adherents of the coding style perhaps here :P
> >
>
> Absolutely!

I disclaim any responsibility for people moaning at you for doing this :P

>
> > > +
> > > +	file = memfd_file_create(name, flags);
> > > +	/* name is not needed beyond this point. */
> >
> > This is nice to highlight! Though it absolutely SUCKS to kmalloc() some
> > memory then instantly discard it like this. But I suppose we have no choice
> > here.
> >
>
> What if instead, we allocate a buffer on the stack and change
> alloc_name() to populate_name() and have it take in a pointer
> to that buffer and join the memfd name prefix and copy the
> name from userspace into that buffer? That avoids having to
> dynamically allocate a buffer that gets freed right away,
> and also gets rid of the cleanup, since that memory will be
> deallocated when we return from the function.

Ah no haha let's not do that, kernel stack space is in short supply. Just a
mild 'hmm'. Let's keep it heap allocated right now, max name size is 255
bytes so yeah.

>
> > >  	kfree(name);
> > > -	return error;
> > > +	if (IS_ERR(file))
> > > +		return PTR_ERR(file);
> > > +
> > > +	fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
> > > +	if (fd >= 0)
> > > +		fd_install(fd, file);
> > > +	else
> > > +		fput(file);
> >
> > You've changed the ordering of this again, and you're not doing anything to
> > free file if something goes wrong with fd allocation? That's a leak no?
> >
>
> file only has one reference to it at this point, and in the else branch,
> or failure case, fput() is invoked to drop the reference on file, which
> will cause it to be freed, so I don't think it's a leak, unless I missed
> something?

You're right, my mistake! Apologies.

>
> > Please reinstate the original ordering.
>
> I'm happy to reinstate the original ordering, but for my knowledge, is
> there a reason as to why fd allocation before file structure allocation
> is preferred instead of the other way around?

Generally I'd prefer us to keep the original ordering (where sane to do
so), as a user might, as insane as it sounds, rely upon a certain error
being returned first, that is literally check the error code for a specific
error, and do something based on that.

As silly as that might sound, this is now out in the wild, and once so we
should try to keep the behaviour as similar as possible.

I think there's some leeway to switching things up if it became so utterly
egregious not to do so, but if it's relatively straightforward we should try.

To me it makes more sense anyway to:

< check if we can reserve an fd >

< allocate the file >

< assign the fd to the file>

But this _may_ just be me... :)

>
> Thanks,
> Isaac
>

[PATCH v2 2/2] mm/memfd: Use strncpy_from_user() to read memfd name

[Isaac J. Manjarres]

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.

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 a9430090bb20..babf6433cf7b 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -394,26 +394,18 @@ static char *memfd_create_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(MFD_NAME_PREFIX_LEN + MFD_NAME_MAX_LEN + 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)) {
+	/* 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

Re: [PATCH v2 2/2] mm/memfd: Use strncpy_from_user() to read memfd name

[Alice Ryhl]

On Tue, Jan 7, 2025 at 7:48 PM Isaac J. Manjarres
<isaacmanjarres@google.com> 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.
>
> Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>

Looks okay to me. One nit below, but:

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> +       /* length does not include terminating zero */
> +       len = strncpy_from_user(name + MFD_NAME_PREFIX_LEN, uname, MFD_NAME_MAX_LEN + 1);

Can we have this comment say "returned length" instead of just
"length"? Or just remove the comment. Initially I thought you were
talking about the last argument, and I was confused as that does
include the nul-terminator.

Alice

Re: [PATCH v2 2/2] mm/memfd: Use strncpy_from_user() to read memfd name

[Isaac Manjarres]

On Wed, Jan 08, 2025 at 02:43:12PM +0100, Alice Ryhl wrote:
> On Tue, Jan 7, 2025 at 7:48 PM Isaac J. Manjarres
> <isaacmanjarres@google.com> 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.
> >
> > Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
> 
> Looks okay to me. One nit below, but:
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 
> > +       /* length does not include terminating zero */
> > +       len = strncpy_from_user(name + MFD_NAME_PREFIX_LEN, uname, MFD_NAME_MAX_LEN + 1);
> 
> Can we have this comment say "returned length" instead of just
> "length"? Or just remove the comment. Initially I thought you were
> talking about the last argument, and I was confused as that does
> include the nul-terminator.
> 
> Alice

Yes, I will update it to say returned length and add your "Reviewed-by"
tag. Thanks for this!

--Isaac

Re: [PATCH v2 2/2] mm/memfd: Use strncpy_from_user() to read memfd name

[Lorenzo Stoakes]

On Tue, Jan 07, 2025 at 10:48:02AM -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().

Thanks this is a good idea!

>
> No functional change.
>
> 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 a9430090bb20..babf6433cf7b 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -394,26 +394,18 @@ static char *memfd_create_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);

See below, but I think we should reinstate this.

> -
> -	name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
> +	name = kmalloc(MFD_NAME_PREFIX_LEN + MFD_NAME_MAX_LEN + 1, GFP_KERNEL);

This seems redundant as:

#define MFD_NAME_MAX_LEN (NAME_MAX - MFD_NAME_PREFIX_LEN)

So MFD_NAME_PREFIX_LEN + MFD_NAME_MAX_LEN + 1
== MFD_NAME_PREFIX_LEN + NAME_MAX - MFD_NAME_PREFIX_LEN + 1
== NAME_MAX + 1

So this should probably just be NAME_MAX + 1.


>  	if (!name)
>  		return ERR_PTR(-ENOMEM);
>
>  	strcpy(name, MFD_NAME_PREFIX);
> -	if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
> +	/* length does not include terminating zero */

Thanks for commenting this! C-style strings are an abomination, so whether or
not something includes the terminating nul(l) is always unclear.

> +	len = strncpy_from_user(name + MFD_NAME_PREFIX_LEN, uname, MFD_NAME_MAX_LEN + 1);

This is sort of nitty, and actually optional honestly, but personally I really
find it a lot clearer to do:

&name[MFD_NAME_PREFIX_LEN]

Here, rather than pointer arithmetic, as it then clearly shows the offset.

> +	if (len < 0) {
>  		error = -EFAULT;

I guess for the purposes of this being user-facing we should keep filtering
the error but yuck. Not your fault though!

>  		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;

I don't think this can ever happen? It just truncates, looking at the code
for strncpy_from_user().

Since we previously returned an error in this instance we should probably
reinstate the removed check for length of input string?

I mean I guess equally the user _could_ change it midway through the copy
and still get truncated, but as far as I'm concerned that'd be an incident
of user insanity which they would naturally have to expect would lead to
undesirable results, so that is fine.

>  		goto err_name;
>  	}
>
> --
> 2.47.1.613.gc27f4b7a9f-goog
>

Re: [PATCH v2 2/2] mm/memfd: Use strncpy_from_user() to read memfd name

[Isaac Manjarres]

On Wed, Jan 08, 2025 at 06:58:00PM +0000, Lorenzo Stoakes wrote:
> On Tue, Jan 07, 2025 at 10:48:02AM -0800, Isaac J. Manjarres wrote:
> > diff --git a/mm/memfd.c b/mm/memfd.c
> > index a9430090bb20..babf6433cf7b 100644
> > --- a/mm/memfd.c
> > +++ b/mm/memfd.c
> > @@ -394,26 +394,18 @@ static char *memfd_create_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);
> 
> See below, but I think we should reinstate this.
> 
> > -
> > -	name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
> > +	name = kmalloc(MFD_NAME_PREFIX_LEN + MFD_NAME_MAX_LEN + 1, GFP_KERNEL);
> 
> This seems redundant as:
> 
> #define MFD_NAME_MAX_LEN (NAME_MAX - MFD_NAME_PREFIX_LEN)
> 
> So MFD_NAME_PREFIX_LEN + MFD_NAME_MAX_LEN + 1
> == MFD_NAME_PREFIX_LEN + NAME_MAX - MFD_NAME_PREFIX_LEN + 1
> == NAME_MAX + 1
> 
> So this should probably just be NAME_MAX + 1.
> 

Thanks, that makes sense to me! I'll update it to NAME_MAX + 1
in v3 of the series.

> > +	len = strncpy_from_user(name + MFD_NAME_PREFIX_LEN, uname, MFD_NAME_MAX_LEN + 1);
> 
> This is sort of nitty, and actually optional honestly, but personally I really
> find it a lot clearer to do:
> 
> &name[MFD_NAME_PREFIX_LEN]
> 
> Here, rather than pointer arithmetic, as it then clearly shows the offset.
> 

That's reasonable; I'll make that change as well.

> >  		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;
> 
> I don't think this can ever happen? It just truncates, looking at the code
> for strncpy_from_user().
> 

I double checked, and this case is possible. The maximum we allow to
strncpy_from_user() to read is MFD_NAME_MAX_LEN + 1 via the count
argument, so that includes the NULL terminator in the userspace buffer.

strncpy_from_user() then returns the length of the string without the
NULL terminator. The check is for just MFD_NAME_MAX_LEN, so this is
meant to catch the case where the string, not including the NULL
terminator, is greater than MFD_NAME_MAX_LEN, which is invalid, as
well as the case where the string becomes malformed/corrupted mid-copy.

Therefore, I think the cases that were caught before are still caught
and handled in the same way. Is there something I'm missing?

Thanks,
Isaac

Re: [PATCH v2 2/2] mm/memfd: Use strncpy_from_user() to read memfd name

[Lorenzo Stoakes]

On Wed, Jan 08, 2025 at 06:15:36PM -0800, Isaac Manjarres wrote:
> On Wed, Jan 08, 2025 at 06:58:00PM +0000, Lorenzo Stoakes wrote:
> > On Tue, Jan 07, 2025 at 10:48:02AM -0800, Isaac J. Manjarres wrote:
> > > diff --git a/mm/memfd.c b/mm/memfd.c
> > > index a9430090bb20..babf6433cf7b 100644
> > > --- a/mm/memfd.c
> > > +++ b/mm/memfd.c
> > > @@ -394,26 +394,18 @@ static char *memfd_create_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);
> >
> > See below, but I think we should reinstate this.
> >
> > > -
> > > -	name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
> > > +	name = kmalloc(MFD_NAME_PREFIX_LEN + MFD_NAME_MAX_LEN + 1, GFP_KERNEL);
> >
> > This seems redundant as:
> >
> > #define MFD_NAME_MAX_LEN (NAME_MAX - MFD_NAME_PREFIX_LEN)
> >
> > So MFD_NAME_PREFIX_LEN + MFD_NAME_MAX_LEN + 1
> > == MFD_NAME_PREFIX_LEN + NAME_MAX - MFD_NAME_PREFIX_LEN + 1
> > == NAME_MAX + 1
> >
> > So this should probably just be NAME_MAX + 1.
> >
>
> Thanks, that makes sense to me! I'll update it to NAME_MAX + 1
> in v3 of the series.
>
> > > +	len = strncpy_from_user(name + MFD_NAME_PREFIX_LEN, uname, MFD_NAME_MAX_LEN + 1);
> >
> > This is sort of nitty, and actually optional honestly, but personally I really
> > find it a lot clearer to do:
> >
> > &name[MFD_NAME_PREFIX_LEN]
> >
> > Here, rather than pointer arithmetic, as it then clearly shows the offset.
> >
>
> That's reasonable; I'll make that change as well.
>

Thanks for above

> > >  		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;
> >
> > I don't think this can ever happen? It just truncates, looking at the code
> > for strncpy_from_user().
> >
>
> I double checked, and this case is possible. The maximum we allow to
> strncpy_from_user() to read is MFD_NAME_MAX_LEN + 1 via the count
> argument, so that includes the NULL terminator in the userspace buffer.
>

> strncpy_from_user() then returns the length of the string without the
> NULL terminator. The check is for just MFD_NAME_MAX_LEN, so this is
> meant to catch the case where the string, not including the NULL
> terminator, is greater than MFD_NAME_MAX_LEN, which is invalid, as
> well as the case where the string becomes malformed/corrupted mid-copy.

Actually you're right :) apologies, I misread the strncpy_from_user()
implementation.

So I think you should be good here - have you tested this scenario in
practice just to confirm?

Cheers!


>
> Therefore, I think the cases that were caught before are still caught
> and handled in the same way. Is there something I'm missing?

No, it seems I probably missed sleep/caffeine at the point I made this
comment ;)

>
> Thanks,
> Isaac

Re: [PATCH v2 2/2] mm/memfd: Use strncpy_from_user() to read memfd name

[Isaac Manjarres]

On Thu, Jan 09, 2025 at 11:31:59AM +0000, Lorenzo Stoakes wrote:
> On Wed, Jan 08, 2025 at 06:15:36PM -0800, Isaac Manjarres wrote:
> > On Wed, Jan 08, 2025 at 06:58:00PM +0000, Lorenzo Stoakes wrote:
> > > On Tue, Jan 07, 2025 at 10:48:02AM -0800, Isaac J. Manjarres wrote:
> > > >  		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;
> > >
> > > I don't think this can ever happen? It just truncates, looking at the code
> > > for strncpy_from_user().
> > >
> >
> > I double checked, and this case is possible. The maximum we allow to
> > strncpy_from_user() to read is MFD_NAME_MAX_LEN + 1 via the count
> > argument, so that includes the NULL terminator in the userspace buffer.
> >
> 
> > strncpy_from_user() then returns the length of the string without the
> > NULL terminator. The check is for just MFD_NAME_MAX_LEN, so this is
> > meant to catch the case where the string, not including the NULL
> > terminator, is greater than MFD_NAME_MAX_LEN, which is invalid, as
> > well as the case where the string becomes malformed/corrupted mid-copy.
> 
> Actually you're right :) apologies, I misread the strncpy_from_user()
> implementation.
> 
> So I think you should be good here - have you tested this scenario in
> practice just to confirm?
> 
> Cheers!

No worries! Yes, I tested this out and confirmed that we do return
-EINVAL in this case before and after this change, so it should be
fine.

Thanks for the review :)! I'll be sending out v3 of this series
shortly.

--Isaac