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

[Isaac Manjarres <isaacmanjarres@google.com>]

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