| Message ID | 93001a9f3f101be0f374080090f9c32df73ca773.1694202430.git.pstanner@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce new wrappers to copy user-arrays | expand |
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>
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
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); > > +}
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 >
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 */
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(+)