[PATCH v2 0/5] Introduce new wrappers to copy user-arrays

[PATCH v2 0/5] Introduce new wrappers to copy user-arrays

[Philipp Stanner]

Hi!

David Airlie suggested that we could implement new wrappers around
(v)memdup_user() for duplicating user arrays.

This small patch series first implements the two new wrapper functions
memdup_array_user() and vmemdup_array_user(). They calculate the
array-sizes safely, i.e., they return an error in case of an overflow.

It then implements the new wrappers in two components in kernel/ and two
in the drm-subsystem.

In total, there are 18 files in the kernel that use (v)memdup_user() to
duplicate arrays. My plan is to provide patches for the other 14
successively once this series has been merged.


Changes since v1:
- Insert new headers alphabetically ordered
- Remove empty lines in functions' docstrings
- Return -EOVERFLOW instead of -EINVAL from wrapper functions


@Andy:
I test-build it for UM on my x86_64. Builds successfully.
A kernel build (localmodconfig) for my Fedora38 @ x86_64 does also boot
fine.

If there is more I can do to verify the early boot stages are fine,
please let me know!

P.

Philipp Stanner (5):
  string.h: add array-wrappers for (v)memdup_user()
  kernel: kexec: copy user-array safely
  kernel: watch_queue: copy user-array safely
  drm_lease.c: copy user-array safely
  drm: vmgfx_surface.c: copy user-array safely

 drivers/gpu/drm/drm_lease.c             |  4 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c |  4 +--
 include/linux/string.h                  | 40 +++++++++++++++++++++++++
 kernel/kexec.c                          |  2 +-
 kernel/watch_queue.c                    |  2 +-
 5 files changed, 46 insertions(+), 6 deletions(-)

-- 
2.41.0

[PATCH v2 1/5] string.h: add array-wrappers for (v)memdup_user()

[Philipp Stanner]

Currently, user array duplications are sometimes done without an
overflow check. Sometimes the checks are done manually; sometimes the
array size is calculated with array_size() and sometimes by calculating
n * size directly in code.

Introduce wrappers for arrays for memdup_user() and vmemdup_user() to
provide a standardized and safe way for duplicating user arrays.

This is both for new code as well as replacing usage of (v)memdup_user()
in existing code that uses, e.g., n * size to calculate array sizes.

Suggested-by: David Airlie <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 include/linux/string.h | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index dbfc66400050..8c9fc76c7154 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -5,7 +5,9 @@
 #include <linux/compiler.h>	/* for inline */
 #include <linux/types.h>	/* for size_t */
 #include <linux/stddef.h>	/* for NULL */
+#include <linux/err.h>		/* for ERR_PTR() */
 #include <linux/errno.h>	/* for E2BIG */
+#include <linux/overflow.h>	/* for check_mul_overflow() */
 #include <linux/stdarg.h>
 #include <uapi/linux/string.h>
 
@@ -14,6 +16,44 @@ extern void *memdup_user(const void __user *, size_t);
 extern void *vmemdup_user(const void __user *, size_t);
 extern void *memdup_user_nul(const void __user *, size_t);
 
+/**
+ * memdup_array_user - duplicate array from user space
+ * @src: source address in user space
+ * @n: number of array members to copy
+ * @size: size of one array member
+ *
+ * Return: an ERR_PTR() on failure. Result is physically
+ * contiguous, to be freed by kfree().
+ */
+static inline void *memdup_array_user(const void __user *src, size_t n, size_t size)
+{
+	size_t nbytes;
+
+	if (unlikely(check_mul_overflow(n, size, &nbytes)))
+		return ERR_PTR(-EOVERFLOW);
+
+	return memdup_user(src, nbytes);
+}
+
+/**
+ * vmemdup_array_user - duplicate array from user space
+ * @src: source address in user space
+ * @n: number of array members to copy
+ * @size: size of one array member
+ *
+ * Return: an ERR_PTR() on failure. Result may be not
+ * physically contiguous. Use kvfree() to free.
+ */
+static inline void *vmemdup_array_user(const void __user *src, size_t n, size_t size)
+{
+	size_t nbytes;
+
+	if (unlikely(check_mul_overflow(n, size, &nbytes)))
+		return ERR_PTR(-EOVERFLOW);
+
+	return vmemdup_user(src, nbytes);
+}
+
 /*
  * Include machine specific inline routines
  */
-- 
2.41.0

Re: [PATCH v2 1/5] string.h: add array-wrappers for (v)memdup_user()

[Andy Shevchenko]

On Fri, Sep 8, 2023 at 11:02 PM Philipp Stanner <pstanner@redhat.com> wrote:
>
> Currently, user array duplications are sometimes done without an
> overflow check. Sometimes the checks are done manually; sometimes the
> array size is calculated with array_size() and sometimes by calculating
> n * size directly in code.
>
> Introduce wrappers for arrays for memdup_user() and vmemdup_user() to
> provide a standardized and safe way for duplicating user arrays.
>
> This is both for new code as well as replacing usage of (v)memdup_user()
> in existing code that uses, e.g., n * size to calculate array sizes.

No objections,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] string.h: add array-wrappers for (v)memdup_user()

[Dan Carpenter]

On Fri, Sep 08, 2023 at 09:59:40PM +0200, Philipp Stanner wrote:
> +static inline void *memdup_array_user(const void __user *src, size_t n, size_t size)
> +{
> +	size_t nbytes;
> +
> +	if (unlikely(check_mul_overflow(n, size, &nbytes)))
> +		return ERR_PTR(-EOVERFLOW);

No need for an unlikely() because check_mul_overflow() already has one
inside.  I feel like -ENOMEM is more traditional but I doubt anyone/userspace
cares.

> +
> +	return memdup_user(src, nbytes);
> +}

regards,
dan carpenter

Re: [PATCH v2 1/5] string.h: add array-wrappers for (v)memdup_user()

[Andy Shevchenko]

On Sat, Sep 16, 2023 at 05:32:42PM +0300, Dan Carpenter wrote:
> On Fri, Sep 08, 2023 at 09:59:40PM +0200, Philipp Stanner wrote:

...

> > +static inline void *memdup_array_user(const void __user *src, size_t n, size_t size)
> > +{
> > +	size_t nbytes;
> > +
> > +	if (unlikely(check_mul_overflow(n, size, &nbytes)))
> > +		return ERR_PTR(-EOVERFLOW);
> 
> No need for an unlikely() because check_mul_overflow() already has one
> inside.

Makes sense.

> I feel like -ENOMEM is more traditional but I doubt anyone/userspace
> cares.

ENOMEM is good for the real allocation calls, here is not the one (the one is
below). Hence ENOMEM is not good candidate above. And whenever functions returns
an error pointer the caller must not assume that it will be only ENOMEM for
allocators.

> > +	return memdup_user(src, nbytes);
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] string.h: add array-wrappers for (v)memdup_user()

[Philipp Stanner]

On Sat, 2023-09-16 at 17:32 +0300, Dan Carpenter wrote:
> On Fri, Sep 08, 2023 at 09:59:40PM +0200, Philipp Stanner wrote:
> > +static inline void *memdup_array_user(const void __user *src,
> > size_t n, size_t size)
> > +{
> > +       size_t nbytes;
> > +
> > +       if (unlikely(check_mul_overflow(n, size, &nbytes)))
> > +               return ERR_PTR(-EOVERFLOW);
> 
> No need for an unlikely() because check_mul_overflow() already has
> one
> inside.

ACK. I overlooked that as it was hidden in the callstack.


>   I feel like -ENOMEM is more traditional but I doubt
> anyone/userspace
> cares.

I agree with Andy here. We're not allocating, so -ENOMEM makes no sense
IMO.


P.

> 
> > +
> > +       return memdup_user(src, nbytes);
> > +}
> 
> regards,
> dan carpenter
> 

[PATCH v2 2/5] kernel: kexec: copy user-array safely

[Philipp Stanner]

Currently, there is no overflow-check with memdup_user().

Use the new function memdup_array_user() instead of memdup_user() for
duplicating the user-space array safely.

Suggested-by: David Airlie <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 kernel/kexec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 107f355eac10..8f35a5a42af8 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -247,7 +247,7 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 		((flags & KEXEC_ARCH_MASK) != KEXEC_ARCH_DEFAULT))
 		return -EINVAL;
 
-	ksegments = memdup_user(segments, nr_segments * sizeof(ksegments[0]));
+	ksegments = memdup_array_user(segments, nr_segments, sizeof(ksegments[0]));
 	if (IS_ERR(ksegments))
 		return PTR_ERR(ksegments);
 
-- 
2.41.0

Re: [PATCH v2 2/5] kernel: kexec: copy user-array safely

[Baoquan He]

On 09/08/23 at 09:59pm, Philipp Stanner wrote:
> Currently, there is no overflow-check with memdup_user().
> 
> Use the new function memdup_array_user() instead of memdup_user() for
> duplicating the user-space array safely.
> 
> Suggested-by: David Airlie <airlied@redhat.com>
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> ---
>  kernel/kexec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 107f355eac10..8f35a5a42af8 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -247,7 +247,7 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
>  		((flags & KEXEC_ARCH_MASK) != KEXEC_ARCH_DEFAULT))
>  		return -EINVAL;
>  
> -	ksegments = memdup_user(segments, nr_segments * sizeof(ksegments[0]));
> +	ksegments = memdup_array_user(segments, nr_segments, sizeof(ksegments[0]));

LGTM,

Acked-by: Baoquan He <bhe@redhat.com>

>  	if (IS_ERR(ksegments))
>  		return PTR_ERR(ksegments);
>  
> -- 
> 2.41.0
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

[PATCH v2 3/5] kernel: watch_queue: copy user-array safely

[Philipp Stanner]

Currently, there is no overflow-check with memdup_user().

Use the new function memdup_array_user() instead of memdup_user() for
duplicating the user-space array safely.

Suggested-by: David Airlie <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 kernel/watch_queue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
index d0b6b390ee42..778b4056700f 100644
--- a/kernel/watch_queue.c
+++ b/kernel/watch_queue.c
@@ -331,7 +331,7 @@ long watch_queue_set_filter(struct pipe_inode_info *pipe,
 	    filter.__reserved != 0)
 		return -EINVAL;
 
-	tf = memdup_user(_filter->filters, filter.nr_filters * sizeof(*tf));
+	tf = memdup_array_user(_filter->filters, filter.nr_filters, sizeof(*tf));
 	if (IS_ERR(tf))
 		return PTR_ERR(tf);
 
-- 
2.41.0

[PATCH v2 4/5] drm_lease.c: copy user-array safely

[Philipp Stanner]

Currently, there is no overflow-check with memdup_user().

Use the new function memdup_array_user() instead of memdup_user() for
duplicating the user-space array safely.

Suggested-by: David Airlie <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/drm_lease.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 150fe1555068..94375c6a5425 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -510,8 +510,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 	/* Handle leased objects, if any */
 	idr_init(&leases);
 	if (object_count != 0) {
-		object_ids = memdup_user(u64_to_user_ptr(cl->object_ids),
-					 array_size(object_count, sizeof(__u32)));
+		object_ids = memdup_array_user(u64_to_user_ptr(cl->object_ids),
+					       object_count, sizeof(__u32));
 		if (IS_ERR(object_ids)) {
 			ret = PTR_ERR(object_ids);
 			idr_destroy(&leases);
-- 
2.41.0

[PATCH v2 5/5] drm: vmgfx_surface.c: copy user-array safely

[Philipp Stanner]

Currently, there is no overflow-check with memdup_user().

Use the new function memdup_array_user() instead of memdup_user() for
duplicating the user-space array safely.

Suggested-by: David Airlie <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 5db403ee8261..9be185b094cb 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -777,9 +777,9 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,
 	       sizeof(metadata->mip_levels));
 	metadata->num_sizes = num_sizes;
 	metadata->sizes =
-		memdup_user((struct drm_vmw_size __user *)(unsigned long)
+		memdup_array_user((struct drm_vmw_size __user *)(unsigned long)
 			    req->size_addr,
-			    sizeof(*metadata->sizes) * metadata->num_sizes);
+			    metadata->num_sizes, sizeof(*metadata->sizes));
 	if (IS_ERR(metadata->sizes)) {
 		ret = PTR_ERR(metadata->sizes);
 		goto out_no_sizes;
-- 
2.41.0

Re: [PATCH v2 0/5] Introduce new wrappers to copy user-arrays

[Kees Cook]

On September 8, 2023 12:59:39 PM PDT, Philipp Stanner <pstanner@redhat.com> wrote:
>Hi!
>
>David Airlie suggested that we could implement new wrappers around
>(v)memdup_user() for duplicating user arrays.
>
>This small patch series first implements the two new wrapper functions
>memdup_array_user() and vmemdup_array_user(). They calculate the
>array-sizes safely, i.e., they return an error in case of an overflow.
>
>It then implements the new wrappers in two components in kernel/ and two
>in the drm-subsystem.
>
>In total, there are 18 files in the kernel that use (v)memdup_user() to
>duplicate arrays. My plan is to provide patches for the other 14
>successively once this series has been merged.
>
>
>Changes since v1:
>- Insert new headers alphabetically ordered
>- Remove empty lines in functions' docstrings
>- Return -EOVERFLOW instead of -EINVAL from wrapper functions
>
>
>@Andy:
>I test-build it for UM on my x86_64. Builds successfully.
>A kernel build (localmodconfig) for my Fedora38 @ x86_64 does also boot
>fine.
>
>If there is more I can do to verify the early boot stages are fine,
>please let me know!
>
>P.
>
>Philipp Stanner (5):
>  string.h: add array-wrappers for (v)memdup_user()
>  kernel: kexec: copy user-array safely
>  kernel: watch_queue: copy user-array safely
>  drm_lease.c: copy user-array safely
>  drm: vmgfx_surface.c: copy user-array safely
>
> drivers/gpu/drm/drm_lease.c             |  4 +--
> drivers/gpu/drm/vmwgfx/vmwgfx_surface.c |  4 +--
> include/linux/string.h                  | 40 +++++++++++++++++++++++++
> kernel/kexec.c                          |  2 +-
> kernel/watch_queue.c                    |  2 +-
> 5 files changed, 46 insertions(+), 6 deletions(-)
>

Nice. For the series:

Reviewed-by: Kees Cook <keescook@chromium.org>



-- 
Kees Cook

Re: [PATCH v2 0/5] Introduce new wrappers to copy user-arrays

[Dave Airlie]

On Tue, 12 Sept 2023 at 11:27, Kees Cook <kees@kernel.org> wrote:
>
> On September 8, 2023 12:59:39 PM PDT, Philipp Stanner <pstanner@redhat.com> wrote:
> >Hi!
> >
> >David Airlie suggested that we could implement new wrappers around
> >(v)memdup_user() for duplicating user arrays.
> >
> >This small patch series first implements the two new wrapper functions
> >memdup_array_user() and vmemdup_array_user(). They calculate the
> >array-sizes safely, i.e., they return an error in case of an overflow.
> >
> >It then implements the new wrappers in two components in kernel/ and two
> >in the drm-subsystem.
> >
> >In total, there are 18 files in the kernel that use (v)memdup_user() to
> >duplicate arrays. My plan is to provide patches for the other 14
> >successively once this series has been merged.
> >
> >
> >Changes since v1:
> >- Insert new headers alphabetically ordered
> >- Remove empty lines in functions' docstrings
> >- Return -EOVERFLOW instead of -EINVAL from wrapper functions
> >
> >
> >@Andy:
> >I test-build it for UM on my x86_64. Builds successfully.
> >A kernel build (localmodconfig) for my Fedora38 @ x86_64 does also boot
> >fine.
> >
> >If there is more I can do to verify the early boot stages are fine,
> >please let me know!
> >
> >P.
> >
> >Philipp Stanner (5):
> >  string.h: add array-wrappers for (v)memdup_user()
> >  kernel: kexec: copy user-array safely
> >  kernel: watch_queue: copy user-array safely
> >  drm_lease.c: copy user-array safely
> >  drm: vmgfx_surface.c: copy user-array safely
> >
> > drivers/gpu/drm/drm_lease.c             |  4 +--
> > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c |  4 +--
> > include/linux/string.h                  | 40 +++++++++++++++++++++++++
> > kernel/kexec.c                          |  2 +-
> > kernel/watch_queue.c                    |  2 +-
> > 5 files changed, 46 insertions(+), 6 deletions(-)
> >
>
> Nice. For the series:
>
> Reviewed-by: Kees Cook <keescook@chromium.org>

Hey Kees,

what tree do you think it would best to land this through? I'm happy
to send the initial set from a drm branch, but also happy to have it
land via someone with a better process.

Dave.

Re: [PATCH v2 0/5] Introduce new wrappers to copy user-arrays

[Kees Cook]

On September 11, 2023 6:55:32 PM PDT, Dave Airlie <airlied@gmail.com> wrote:
>On Tue, 12 Sept 2023 at 11:27, Kees Cook <kees@kernel.org> wrote:
>>
>> On September 8, 2023 12:59:39 PM PDT, Philipp Stanner <pstanner@redhat.com> wrote:
>> >Hi!
>> >
>> >David Airlie suggested that we could implement new wrappers around
>> >(v)memdup_user() for duplicating user arrays.
>> >
>> >This small patch series first implements the two new wrapper functions
>> >memdup_array_user() and vmemdup_array_user(). They calculate the
>> >array-sizes safely, i.e., they return an error in case of an overflow.
>> >
>> >It then implements the new wrappers in two components in kernel/ and two
>> >in the drm-subsystem.
>> >
>> >In total, there are 18 files in the kernel that use (v)memdup_user() to
>> >duplicate arrays. My plan is to provide patches for the other 14
>> >successively once this series has been merged.
>> >
>> >
>> >Changes since v1:
>> >- Insert new headers alphabetically ordered
>> >- Remove empty lines in functions' docstrings
>> >- Return -EOVERFLOW instead of -EINVAL from wrapper functions
>> >
>> >
>> >@Andy:
>> >I test-build it for UM on my x86_64. Builds successfully.
>> >A kernel build (localmodconfig) for my Fedora38 @ x86_64 does also boot
>> >fine.
>> >
>> >If there is more I can do to verify the early boot stages are fine,
>> >please let me know!
>> >
>> >P.
>> >
>> >Philipp Stanner (5):
>> >  string.h: add array-wrappers for (v)memdup_user()
>> >  kernel: kexec: copy user-array safely
>> >  kernel: watch_queue: copy user-array safely
>> >  drm_lease.c: copy user-array safely
>> >  drm: vmgfx_surface.c: copy user-array safely
>> >
>> > drivers/gpu/drm/drm_lease.c             |  4 +--
>> > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c |  4 +--
>> > include/linux/string.h                  | 40 +++++++++++++++++++++++++
>> > kernel/kexec.c                          |  2 +-
>> > kernel/watch_queue.c                    |  2 +-
>> > 5 files changed, 46 insertions(+), 6 deletions(-)
>> >
>>
>> Nice. For the series:
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>
>Hey Kees,
>
>what tree do you think it would best to land this through? I'm happy
>to send the initial set from a drm branch, but also happy to have it
>land via someone with a better process.

Feel free to take it via drm. Usually string.h doesn't get a lot of changes (and even then it's normally additive) so conflicts are rare/easy. :)

-Kees


-- 
Kees Cook

Re: [PATCH v2 0/5] Introduce new wrappers to copy user-arrays

[Zack Rusin]

On Fri, 2023-09-08 at 21:59 +0200, Philipp Stanner wrote:
> Hi!
> 
> David Airlie suggested that we could implement new wrappers around
> (v)memdup_user() for duplicating user arrays.
> 
> This small patch series first implements the two new wrapper functions
> memdup_array_user() and vmemdup_array_user(). They calculate the
> array-sizes safely, i.e., they return an error in case of an overflow.
> 
> It then implements the new wrappers in two components in kernel/ and two
> in the drm-subsystem.
> 
> In total, there are 18 files in the kernel that use (v)memdup_user() to
> duplicate arrays. My plan is to provide patches for the other 14
> successively once this series has been merged.
> 
> 
> Changes since v1:
> - Insert new headers alphabetically ordered
> - Remove empty lines in functions' docstrings
> - Return -EOVERFLOW instead of -EINVAL from wrapper functions
> 
> 
> @Andy:
> I test-build it for UM on my x86_64. Builds successfully.
> A kernel build (localmodconfig) for my Fedora38 @ x86_64 does also boot
> fine.
> 
> If there is more I can do to verify the early boot stages are fine,
> please let me know!
> 
> P.
> 
> Philipp Stanner (5):
>   string.h: add array-wrappers for (v)memdup_user()
>   kernel: kexec: copy user-array safely
>   kernel: watch_queue: copy user-array safely
>   drm_lease.c: copy user-array safely
>   drm: vmgfx_surface.c: copy user-array safely
> 
>  drivers/gpu/drm/drm_lease.c             |  4 +--
>  drivers/gpu/drm/vmwgfx/vmwgfx_surface.c |  4 +--
>  include/linux/string.h                  | 40 +++++++++++++++++++++++++
>  kernel/kexec.c                          |  2 +-
>  kernel/watch_queue.c                    |  2 +-
>  5 files changed, 46 insertions(+), 6 deletions(-)
> 

Series, and in particular the vmwgfx changes, look good to me.

Reviewed-by: Zack Rusin <zackr@vmware.com>