Re: [PATCH] proc: use vmalloc for our kernel buffer
On 8/13/20 12:19 PM, David Laight wrote: From: Josef Bacik Sent: 13 August 2020 15:53 sysctl: pass kernel pointers to ->proc_handler we have been pre-allocating a buffer to copy the data from the proc handlers into, and then copying that to userspace. The problem is this just blind kmalloc()'s the buffer size passed in from the read, which in the case of our 'cat' binary was 64kib. Order-4 allocations are not awesome, and since we can potentially allocate up to our maximum order, use vmalloc for these buffers. What happens if I run 'dd bs=16M ...' ? David /* don't even try if the size is too large */ error = -ENOMEM; if (count >= KMALLOC_MAX_SIZE) goto out; is above this code, thanks, Josef
Re: [PATCH] proc: use vmalloc for our kernel buffer
On Thu, Aug 13, 2020 at 04:19:27PM +, David Laight wrote: > From: Josef Bacik > > Sent: 13 August 2020 15:53 > > > > sysctl: pass kernel pointers to ->proc_handler > > > > we have been pre-allocating a buffer to copy the data from the proc > > handlers into, and then copying that to userspace. The problem is this > > just blind kmalloc()'s the buffer size passed in from the read, which in > > the case of our 'cat' binary was 64kib. Order-4 allocations are not > > awesome, and since we can potentially allocate up to our maximum order, > > use vmalloc for these buffers. > > What happens if I run 'dd bs=16M ...' ? Try it.
RE: [PATCH] proc: use vmalloc for our kernel buffer
From: Josef Bacik > Sent: 13 August 2020 15:53 > > sysctl: pass kernel pointers to ->proc_handler > > we have been pre-allocating a buffer to copy the data from the proc > handlers into, and then copying that to userspace. The problem is this > just blind kmalloc()'s the buffer size passed in from the read, which in > the case of our 'cat' binary was 64kib. Order-4 allocations are not > awesome, and since we can potentially allocate up to our maximum order, > use vmalloc for these buffers. What happens if I run 'dd bs=16M ...' ? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] proc: use vmalloc for our kernel buffer
On 8/13/20 10:59 AM, Matthew Wilcox wrote:
On Thu, Aug 13, 2020 at 10:53:05AM -0400, Josef Bacik wrote:
+/**
+ * vmemdup_user - duplicate memory region from user space and NUL-terminate
vmemdup_user_nul()
+void *vmemdup_user_nul(const void __user *src, size_t len)
+{
+ void *p;
+
+ p = kvmalloc(len, GFP_USER);
len+1, shirley?
+ if (!p)
+ return ERR_PTR(-ENOMEM);
+
+ if (copy_from_user(p, src, len)) {
+ kvfree(p);
+ return ERR_PTR(-EFAULT);
+ }
I think you forgot
p[len] = '\0';
Sweet lord I need more sleep, my bad. Thanks,
Josef
Re: [PATCH] proc: use vmalloc for our kernel buffer
On Thu, Aug 13, 2020 at 10:53:05AM -0400, Josef Bacik wrote:
> +/**
> + * vmemdup_user - duplicate memory region from user space and NUL-terminate
vmemdup_user_nul()
> +void *vmemdup_user_nul(const void __user *src, size_t len)
> +{
> + void *p;
> +
> + p = kvmalloc(len, GFP_USER);
len+1, shirley?
> + if (!p)
> + return ERR_PTR(-ENOMEM);
> +
> + if (copy_from_user(p, src, len)) {
> + kvfree(p);
> + return ERR_PTR(-EFAULT);
> + }
I think you forgot
p[len] = '\0';
> + return p;
> +}
> +EXPORT_SYMBOL(vmemdup_user_nul);
> +
> /**
> * strndup_user - duplicate an existing string from user space
> * @s: The string to duplicate
> --
> 2.24.1
>
[PATCH] proc: use vmalloc for our kernel buffer
Since
sysctl: pass kernel pointers to ->proc_handler
we have been pre-allocating a buffer to copy the data from the proc
handlers into, and then copying that to userspace. The problem is this
just blind kmalloc()'s the buffer size passed in from the read, which in
the case of our 'cat' binary was 64kib. Order-4 allocations are not
awesome, and since we can potentially allocate up to our maximum order,
use vmalloc for these buffers.
Fixes: 32927393dc1c ("sysctl: pass kernel pointers to ->proc_handler")
Signed-off-by: Josef Bacik
---
fs/proc/proc_sysctl.c | 6 +++---
include/linux/string.h | 1 +
mm/util.c | 26 ++
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 6c1166ccdaea..207ac6e6e028 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -571,13 +571,13 @@ static ssize_t proc_sys_call_handler(struct file *filp,
void __user *ubuf,
goto out;
if (write) {
- kbuf = memdup_user_nul(ubuf, count);
+ kbuf = vmemdup_user_nul(ubuf, count);
if (IS_ERR(kbuf)) {
error = PTR_ERR(kbuf);
goto out;
}
} else {
- kbuf = kzalloc(count, GFP_KERNEL);
+ kbuf = kvzalloc(count, GFP_KERNEL);
if (!kbuf)
goto out;
}
@@ -600,7 +600,7 @@ static ssize_t proc_sys_call_handler(struct file *filp,
void __user *ubuf,
error = count;
out_free_buf:
- kfree(kbuf);
+ kvfree(kbuf);
out:
sysctl_head_finish(head);
diff --git a/include/linux/string.h b/include/linux/string.h
index 9b7a0632e87a..aee3689fb865 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -12,6 +12,7 @@
extern char *strndup_user(const char __user *, long);
extern void *memdup_user(const void __user *, size_t);
extern void *vmemdup_user(const void __user *, size_t);
+extern void *vmemdup_user_nul(const void __user *, size_t);
extern void *memdup_user_nul(const void __user *, size_t);
/*
diff --git a/mm/util.c b/mm/util.c
index 5ef378a2a038..4de3b4b0f358 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -208,6 +208,32 @@ void *vmemdup_user(const void __user *src, size_t len)
}
EXPORT_SYMBOL(vmemdup_user);
+/**
+ * vmemdup_user - duplicate memory region from user space and NUL-terminate
+ *
+ * @src: source address in user space
+ * @len: number of bytes to copy
+ *
+ * Return: an ERR_PTR() on failure. Result may be not
+ * physically contiguous. Use kvfree() to free.
+ */
+void *vmemdup_user_nul(const void __user *src, size_t len)
+{
+ void *p;
+
+ p = kvmalloc(len, GFP_USER);
+ if (!p)
+ return ERR_PTR(-ENOMEM);
+
+ if (copy_from_user(p, src, len)) {
+ kvfree(p);
+ return ERR_PTR(-EFAULT);
+ }
+
+ return p;
+}
+EXPORT_SYMBOL(vmemdup_user_nul);
+
/**
* strndup_user - duplicate an existing string from user space
* @s: The string to duplicate
--
2.24.1
