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
