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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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