| Message ID | 20250107184804.4074147-3-isaacmanjarres@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanup for memfd_create() | expand |
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
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
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 >
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
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
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
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; }
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(-)