[PATCH 0/6] Some buffer management fixes for proc

[PATCH 0/6] Some buffer management fixes for proc

[Josef Bacik]

This initialy started with

  [PATCH 1/6] proc: use vmalloc for our kernel buffer

Which came about because we were getting page alloc failures when cat tried to
do a read with a 64kib buffer, triggering an order 4 allocation.  We need to
switch to kvmalloc for this buffer to avoid these high order allocations.  Then
Christoph suggested renaming vmemdup_user to kvmemdup_user, so then we have this

  [PATCH 2/6] tree-wide: rename vmemdup_user to kvmemdup_user

And then finally Viro noticed that if we allocate an extra byte for the NULL
terminator then we can use scnprintf() in a few places, and thus the next set of
patches

  [PATCH 3/6] proc: allocate count + 1 for our read buffer
  [PATCH 4/6] sysctl: make proc_put_long() use scnprintf
  [PATCH 5/6] parport: rework procfs handlers to take advantage of the
  [PATCH 6/6] sunrpc: rework proc handlers to take advantage of the new

There's one case that I didn't convert, _proc_do_string, and that's because it's
one of the few places that takes into account ppos, and so we'll skip forward in
the string we're provided with from the caller.  In this case it makes sense to
just leave it the way it is.  I'm pretty sure I caught all the other people who
directly mess with the buffer, but there's around 800 ->proc_handler's, and my
eyes started to glaze over after a while.

Josef

[PATCH 1/6] proc: use vmalloc for our kernel buffer

[Josef Bacik]

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 <josef@toxicpanda.com>
---
 fs/proc/proc_sysctl.c  |  6 +++---
 include/linux/string.h |  1 +
 mm/util.c              | 27 +++++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 6c1166ccdaea..8e19bad83b45 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 = kvmemdup_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..21bb6d3d88c4 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 *kvmemdup_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..cf454d57d3e2 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -208,6 +208,33 @@ void *vmemdup_user(const void __user *src, size_t len)
 }
 EXPORT_SYMBOL(vmemdup_user);
 
+/**
+ * kvmemdup_user_nul - 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 *kvmemdup_user_nul(const void __user *src, size_t len)
+{
+	char *p;
+
+	p = kvmalloc(len + 1, GFP_USER);
+	if (!p)
+		return ERR_PTR(-ENOMEM);
+
+	if (copy_from_user(p, src, len)) {
+		kvfree(p);
+		return ERR_PTR(-EFAULT);
+	}
+	p[len] = '\0';
+
+	return p;
+}
+EXPORT_SYMBOL(kvmemdup_user_nul);
+
 /**
  * strndup_user - duplicate an existing string from user space
  * @s: The string to duplicate
-- 
2.24.1

Re: [PATCH 1/6] proc: use vmalloc for our kernel buffer

[Christoph Hellwig]

On Thu, Aug 13, 2020 at 05:04:06PM -0400, Josef Bacik wrote:
> 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.

Maybe the subject should read ".. also use vmalloc" as we still default
to kmalloc for small allocations?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/6] tree-wide: rename vmemdup_user to kvmemdup_user

[Josef Bacik]

This helper uses kvmalloc, not vmalloc, so rename it to kvmemdup_user to
make it clear we're using kvmalloc() and will need to use kvfree().

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 arch/x86/kvm/cpuid.c                   | 6 +++---
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 2 +-
 drivers/tty/vt/consolemap.c            | 2 +-
 include/linux/string.h                 | 2 +-
 mm/util.c                              | 6 +++---
 sound/core/control.c                   | 4 ++--
 virt/kvm/kvm_main.c                    | 4 ++--
 7 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 3fd6eec202d7..22834ea499ee 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -200,9 +200,9 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	if (cpuid->nent > KVM_MAX_CPUID_ENTRIES)
 		goto out;
 	if (cpuid->nent) {
-		cpuid_entries = vmemdup_user(entries,
-					     array_size(sizeof(struct kvm_cpuid_entry),
-							cpuid->nent));
+		cpuid_entries = kvmemdup_user(entries,
+					      array_size(sizeof(struct kvm_cpuid_entry),
+							 cpuid->nent));
 		if (IS_ERR(cpuid_entries)) {
 			r = PTR_ERR(cpuid_entries);
 			goto out;
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 7a2430e34e00..c2f973aa3680 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -147,7 +147,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 		bo_handles = NULL;
 	}
 
-	buf = vmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
+	buf = kvmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
 	if (IS_ERR(buf)) {
 		ret = PTR_ERR(buf);
 		goto out_unused_fd;
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 5947b54d92be..2cffa8b3c74b 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -542,7 +542,7 @@ int con_set_unimap(struct vc_data *vc, ushort ct, struct unipair __user *list)
 	if (!ct)
 		return 0;
 
-	unilist = vmemdup_user(list, array_size(sizeof(struct unipair), ct));
+	unilist = kvmemdup_user(list, array_size(sizeof(struct unipair), ct));
 	if (IS_ERR(unilist))
 		return PTR_ERR(unilist);
 
diff --git a/include/linux/string.h b/include/linux/string.h
index 21bb6d3d88c4..a6f7218124a0 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -11,7 +11,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 *kvmemdup_user(const void __user *, size_t);
 extern void *kvmemdup_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 cf454d57d3e2..f434634b6ba3 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -183,7 +183,7 @@ void *memdup_user(const void __user *src, size_t len)
 EXPORT_SYMBOL(memdup_user);
 
 /**
- * vmemdup_user - duplicate memory region from user space
+ * kvmemdup_user - duplicate memory region from user space
  *
  * @src: source address in user space
  * @len: number of bytes to copy
@@ -191,7 +191,7 @@ EXPORT_SYMBOL(memdup_user);
  * Return: an ERR_PTR() on failure.  Result may be not
  * physically contiguous.  Use kvfree() to free.
  */
-void *vmemdup_user(const void __user *src, size_t len)
+void *kvmemdup_user(const void __user *src, size_t len)
 {
 	void *p;
 
@@ -206,7 +206,7 @@ void *vmemdup_user(const void __user *src, size_t len)
 
 	return p;
 }
-EXPORT_SYMBOL(vmemdup_user);
+EXPORT_SYMBOL(kvmemdup_user);
 
 /**
  * kvmemdup_user_nul - duplicate memory region from user space and NUL-terminate
diff --git a/sound/core/control.c b/sound/core/control.c
index aa0c0cf182af..b712f4d261de 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1297,7 +1297,7 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf,
 	if (size > 1024 * 128)	/* sane value */
 		return -EINVAL;
 
-	container = vmemdup_user(buf, size);
+	container = kvmemdup_user(buf, size);
 	if (IS_ERR(container))
 		return PTR_ERR(container);
 
@@ -1365,7 +1365,7 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
 	if (ue->info.value.enumerated.names_length > 64 * 1024)
 		return -EINVAL;
 
-	names = vmemdup_user((const void __user *)user_ptrval,
+	names = kvmemdup_user((const void __user *)user_ptrval,
 		ue->info.value.enumerated.names_length);
 	if (IS_ERR(names))
 		return PTR_ERR(names);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 737666db02de..1111780ccefd 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3814,8 +3814,8 @@ static long kvm_vm_ioctl(struct file *filp,
 			goto out;
 		if (routing.nr) {
 			urouting = argp;
-			entries = vmemdup_user(urouting->entries,
-					       array_size(sizeof(*entries),
+			entries = kvmemdup_user(urouting->entries,
+						array_size(sizeof(*entries),
 							  routing.nr));
 			if (IS_ERR(entries)) {
 				r = PTR_ERR(entries);
-- 
2.24.1

Re: [PATCH 2/6] tree-wide: rename vmemdup_user to kvmemdup_user

[Christoph Hellwig]

On Thu, Aug 13, 2020 at 05:04:07PM -0400, Josef Bacik wrote:
> This helper uses kvmalloc, not vmalloc, so rename it to kvmemdup_user to
> make it clear we're using kvmalloc() and will need to use kvfree().
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/6] proc: allocate count + 1 for our read buffer

[Josef Bacik]

Al suggested that if we allocate enough space to add in the '\0'
character at the end of our strings, we could just use scnprintf() in
our ->proc_handler functions without having to be fancy about keeping
track of space.  There are a lot of these handlers, so the follow ups
will be separate, but start with allocating the extra byte to handle the
null termination of strings.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/proc/proc_sysctl.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 8e19bad83b45..446e7a949025 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -548,6 +548,7 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *ubuf,
 	struct ctl_table *table = PROC_I(inode)->sysctl_entry;
 	void *kbuf;
 	ssize_t error;
+	size_t orig_count = count;
 
 	if (IS_ERR(head))
 		return PTR_ERR(head);
@@ -577,9 +578,23 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *ubuf,
 			goto out;
 		}
 	} else {
-		kbuf = kvzalloc(count, GFP_KERNEL);
+		/*
+		 * To make our lives easier in ->proc_handler, we allocate an
+		 * extra byte to allow us to use scnprintf() for handling the
+		 * buffer output.  This works properly because scnprintf() will
+		 * only return the number of bytes that it was able to write
+		 * out, _NOT_ including the NULL byte.  This means the handler's
+		 * will only ever return a maximum of count as what they've
+		 * copied.
+		 *
+		 * HOWEVER, we do not assume that ->proc_handlers are without
+		 * bugs, so further down we'll do an extra check to make sure
+		 * that count isn't larger than the orig_count.
+		 */
+		kbuf = kvzalloc(count + 1, GFP_KERNEL);
 		if (!kbuf)
 			goto out;
+		count += 1;
 	}
 
 	error = BPF_CGROUP_RUN_PROG_SYSCTL(head, table, write, &kbuf, &count,
@@ -593,6 +608,13 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *ubuf,
 		goto out_free_buf;
 
 	if (!write) {
+		/*
+		 * This shouldn't happen, but those are the last words before
+		 * somebody adds a security vulnerability, so just make sure
+		 * that count isn't larger than orig_count.
+		 */
+		if (count > orig_count)
+			count = orig_count;
 		error = -EFAULT;
 		if (copy_to_user(ubuf, kbuf, count))
 			goto out_free_buf;
-- 
2.24.1

Re: [PATCH 3/6] proc: allocate count + 1 for our read buffer

[Christoph Hellwig]

On Thu, Aug 13, 2020 at 05:04:08PM -0400, Josef Bacik wrote:
> Al suggested that if we allocate enough space to add in the '\0'
> character at the end of our strings, we could just use scnprintf() in
> our ->proc_handler functions without having to be fancy about keeping
> track of space.  There are a lot of these handlers, so the follow ups
> will be separate, but start with allocating the extra byte to handle the
> null termination of strings.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/6] sysctl: make proc_put_long() use scnprintf

[Josef Bacik]

Now that we're passing down a kernel buffer with enough space to account
for an extra NULL terminator, go ahead and use scnprintf() to print out
a long in proc_put_long().  count here includes NULL terminator slot in
the buffer, so we will get the correct behavior we're looking for.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 kernel/sysctl.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 287862f91717..d8cc8737f58f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -484,6 +484,7 @@ static int proc_get_long(char **buf, size_t *size,
 
 	return 0;
 }
+#undef TMPBUFLEN
 
 /**
  * proc_put_long - converts an integer to a decimal ASCII formatted string
@@ -498,18 +499,12 @@ static int proc_get_long(char **buf, size_t *size,
  */
 static void proc_put_long(void **buf, size_t *size, unsigned long val, bool neg)
 {
-	int len;
-	char tmp[TMPBUFLEN], *p = tmp;
+	size_t ret;
 
-	sprintf(p, "%s%lu", neg ? "-" : "", val);
-	len = strlen(tmp);
-	if (len > *size)
-		len = *size;
-	memcpy(*buf, tmp, len);
-	*size -= len;
-	*buf += len;
+	ret = scnprintf(*buf, *size, "%s%lu", neg ? "-" : "", val);
+	*size -= ret;
+	*buf += ret;
 }
-#undef TMPBUFLEN
 
 static void proc_put_char(void **buf, size_t *size, char c)
 {
-- 
2.24.1

Re: [PATCH 4/6] sysctl: make proc_put_long() use scnprintf

[Christoph Hellwig]

On Thu, Aug 13, 2020 at 05:04:09PM -0400, Josef Bacik wrote:
> Now that we're passing down a kernel buffer with enough space to account
> for an extra NULL terminator, go ahead and use scnprintf() to print out
> a long in proc_put_long().  count here includes NULL terminator slot in
> the buffer, so we will get the correct behavior we're looking for.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 5/6] parport: rework procfs handlers to take advantage of the new buffer

[Josef Bacik]

The buffer coming from higher up the stack has an extra byte to handle
the NULL terminator in the string.  Instead of using a temporary buffer
to sprintf into and then copying into the buffer, just scnprintf
directly into the buffer and update lenp as appropriate.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 drivers/parport/procfs.c | 108 +++++++++++++--------------------------
 1 file changed, 36 insertions(+), 72 deletions(-)

diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index d740eba3c099..453d035ad5f6 100644
--- a/drivers/parport/procfs.c
+++ b/drivers/parport/procfs.c
@@ -37,9 +37,8 @@ static int do_active_device(struct ctl_table *table, int write,
 		      void *result, size_t *lenp, loff_t *ppos)
 {
 	struct parport *port = (struct parport *)table->extra1;
-	char buffer[256];
 	struct pardevice *dev;
-	int len = 0;
+	size_t ret = 0;
 
 	if (write)		/* can't happen anyway */
 		return -EACCES;
@@ -48,24 +47,19 @@ static int do_active_device(struct ctl_table *table, int write,
 		*lenp = 0;
 		return 0;
 	}
-	
+
 	for (dev = port->devices; dev ; dev = dev->next) {
 		if(dev == port->cad) {
-			len += sprintf(buffer, "%s\n", dev->name);
+			ret += scnprintf(result + ret, *lenp - ret, "%s\n",
+					 dev->name);
 		}
 	}
 
-	if(!len) {
-		len += sprintf(buffer, "%s\n", "none");
-	}
-
-	if (len > *lenp)
-		len = *lenp;
-	else
-		*lenp = len;
+	if (!ret)
+		ret = scnprintf(result, *lenp, "%s\n", "none");
 
-	*ppos += len;
-	memcpy(result, buffer, len);
+	*lenp = ret;
+	*ppos += ret;
 	return 0;
 }
 
@@ -75,8 +69,7 @@ static int do_autoprobe(struct ctl_table *table, int write,
 {
 	struct parport_device_info *info = table->extra2;
 	const char *str;
-	char buffer[256];
-	int len = 0;
+	size_t ret = 0;
 
 	if (write) /* permissions stop this */
 		return -EACCES;
@@ -85,30 +78,24 @@ static int do_autoprobe(struct ctl_table *table, int write,
 		*lenp = 0;
 		return 0;
 	}
-	
+
 	if ((str = info->class_name) != NULL)
-		len += sprintf (buffer + len, "CLASS:%s;\n", str);
+		ret += scnprintf(result + ret, *lenp - ret, "CLASS:%s;\n", str);
 
 	if ((str = info->model) != NULL)
-		len += sprintf (buffer + len, "MODEL:%s;\n", str);
+		ret += scnprintf(result + ret, *lenp - ret, "MODEL:%s;\n", str);
 
 	if ((str = info->mfr) != NULL)
-		len += sprintf (buffer + len, "MANUFACTURER:%s;\n", str);
+		ret += scnprintf(result + ret, *lenp - ret, "MANUFACTURER:%s;\n", str);
 
 	if ((str = info->description) != NULL)
-		len += sprintf (buffer + len, "DESCRIPTION:%s;\n", str);
+		ret += scnprintf(result + ret, *lenp - ret, "DESCRIPTION:%s;\n", str);
 
 	if ((str = info->cmdset) != NULL)
-		len += sprintf (buffer + len, "COMMAND SET:%s;\n", str);
-
-	if (len > *lenp)
-		len = *lenp;
-	else
-		*lenp = len;
+		ret += scnprintf(result + ret, *lenp - ret, "COMMAND SET:%s;\n", str);
 
-	*ppos += len;
-
-	memcpy(result, buffer, len);
+	*lenp = ret;
+	*ppos += ret;
 	return 0;
 }
 #endif /* IEEE1284.3 support. */
@@ -117,8 +104,7 @@ static int do_hardware_base_addr(struct ctl_table *table, int write,
 				 void *result, size_t *lenp, loff_t *ppos)
 {
 	struct parport *port = (struct parport *)table->extra1;
-	char buffer[20];
-	int len = 0;
+	size_t ret;
 
 	if (*ppos) {
 		*lenp = 0;
@@ -128,15 +114,10 @@ static int do_hardware_base_addr(struct ctl_table *table, int write,
 	if (write) /* permissions prevent this anyway */
 		return -EACCES;
 
-	len += sprintf (buffer, "%lu\t%lu\n", port->base, port->base_hi);
-
-	if (len > *lenp)
-		len = *lenp;
-	else
-		*lenp = len;
-
-	*ppos += len;
-	memcpy(result, buffer, len);
+	ret = scnprintf(result, *lenp, "%lu\t%lu\n", port->base,
+			port->base_hi);
+	*lenp = ret;
+	*ppos += ret;
 	return 0;
 }
 
@@ -144,8 +125,7 @@ static int do_hardware_irq(struct ctl_table *table, int write,
 			   void *result, size_t *lenp, loff_t *ppos)
 {
 	struct parport *port = (struct parport *)table->extra1;
-	char buffer[20];
-	int len = 0;
+	size_t ret;
 
 	if (*ppos) {
 		*lenp = 0;
@@ -155,15 +135,10 @@ static int do_hardware_irq(struct ctl_table *table, int write,
 	if (write) /* permissions prevent this anyway */
 		return -EACCES;
 
-	len += sprintf (buffer, "%d\n", port->irq);
+	ret = scnprintf(result, *lenp, "%d\n", port->irq);
 
-	if (len > *lenp)
-		len = *lenp;
-	else
-		*lenp = len;
-
-	*ppos += len;
-	memcpy(result, buffer, len);
+	*lenp = ret;
+	*ppos += ret;
 	return 0;
 }
 
@@ -171,8 +146,7 @@ static int do_hardware_dma(struct ctl_table *table, int write,
 			   void *result, size_t *lenp, loff_t *ppos)
 {
 	struct parport *port = (struct parport *)table->extra1;
-	char buffer[20];
-	int len = 0;
+	size_t ret;
 
 	if (*ppos) {
 		*lenp = 0;
@@ -182,15 +156,10 @@ static int do_hardware_dma(struct ctl_table *table, int write,
 	if (write) /* permissions prevent this anyway */
 		return -EACCES;
 
-	len += sprintf (buffer, "%d\n", port->dma);
-
-	if (len > *lenp)
-		len = *lenp;
-	else
-		*lenp = len;
+	ret = scnprintf(result, *lenp, "%d\n", port->dma);
 
-	*ppos += len;
-	memcpy(result, buffer, len);
+	*lenp = ret;
+	*ppos += ret;
 	return 0;
 }
 
@@ -198,8 +167,7 @@ static int do_hardware_modes(struct ctl_table *table, int write,
 			     void *result, size_t *lenp, loff_t *ppos)
 {
 	struct parport *port = (struct parport *)table->extra1;
-	char buffer[40];
-	int len = 0;
+	size_t ret = 0;
 
 	if (*ppos) {
 		*lenp = 0;
@@ -213,7 +181,8 @@ static int do_hardware_modes(struct ctl_table *table, int write,
 #define printmode(x)							\
 do {									\
 	if (port->modes & PARPORT_MODE_##x)				\
-		len += sprintf(buffer + len, "%s%s", f++ ? "," : "", #x); \
+		ret += scnprintf(result + ret, *lenp - ret,		\
+				 "%s%s", f++ ? "," : "", #x);		\
 } while (0)
 		int f = 0;
 		printmode(PCSPP);
@@ -224,15 +193,10 @@ do {									\
 		printmode(DMA);
 #undef printmode
 	}
-	buffer[len++] = '\n';
-
-	if (len > *lenp)
-		len = *lenp;
-	else
-		*lenp = len;
+	ret += scnprintf(result + ret, *lenp - ret, "\n");
 
-	*ppos += len;
-	memcpy(result, buffer, len);
+	*lenp = ret;
+	*ppos += ret;
 	return 0;
 }
 
-- 
2.24.1

Re: [PATCH 5/6] parport: rework procfs handlers to take advantage of the new buffer

[Christoph Hellwig]

On Thu, Aug 13, 2020 at 05:04:10PM -0400, Josef Bacik wrote:
> The buffer coming from higher up the stack has an extra byte to handle
> the NULL terminator in the string.  Instead of using a temporary buffer
> to sprintf into and then copying into the buffer, just scnprintf
> directly into the buffer and update lenp as appropriate.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 6/6] sunrpc: rework proc handlers to take advantage of the new buffer

[Josef Bacik]

Now that we're allocating an extra slot for the NULL terminated string,
use scnprintf() and write directly into the buffer.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 net/sunrpc/sysctl.c            | 10 ++--------
 net/sunrpc/xprtrdma/svc_rdma.c | 16 ++--------------
 2 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
index 999eee1ed61c..31ed530d9846 100644
--- a/net/sunrpc/sysctl.c
+++ b/net/sunrpc/sysctl.c
@@ -117,14 +117,8 @@ proc_dodebug(struct ctl_table *table, int write, void *buffer, size_t *lenp,
 		if (strcmp(table->procname, "rpc_debug") == 0)
 			rpc_show_tasks(&init_net);
 	} else {
-		len = sprintf(tmpbuf, "0x%04x", *(unsigned int *) table->data);
-		if (len > left)
-			len = left;
-		memcpy(buffer, tmpbuf, len);
-		if ((left -= len) > 0) {
-			*((char *)buffer + len) = '\n';
-			left--;
-		}
+		len = scnprintf(buffer, *lenp, "0x%04x\n", *(unsigned int *) table->data);
+		left -= len;
 	}
 
 done:
diff --git a/net/sunrpc/xprtrdma/svc_rdma.c b/net/sunrpc/xprtrdma/svc_rdma.c
index 526da5d4710b..9b3a113598af 100644
--- a/net/sunrpc/xprtrdma/svc_rdma.c
+++ b/net/sunrpc/xprtrdma/svc_rdma.c
@@ -90,20 +90,8 @@ static int read_reset_stat(struct ctl_table *table, int write,
 	if (write)
 		atomic_set(stat, 0);
 	else {
-		char str_buf[32];
-		int len = snprintf(str_buf, 32, "%d\n", atomic_read(stat));
-		if (len >= 32)
-			return -EFAULT;
-		len = strlen(str_buf);
-		if (*ppos > len) {
-			*lenp = 0;
-			return 0;
-		}
-		len -= *ppos;
-		if (len > *lenp)
-			len = *lenp;
-		if (len)
-			memcpy(buffer, str_buf, len);
+		size_t len = scnprintf(buffer, *lenp, "%d\n",
+				       atomic_read(stat));
 		*lenp = len;
 		*ppos += len;
 	}
-- 
2.24.1

Re: [PATCH 6/6] sunrpc: rework proc handlers to take advantage of the new buffer

[Christoph Hellwig]

On Thu, Aug 13, 2020 at 05:04:11PM -0400, Josef Bacik wrote:
> +		len = scnprintf(buffer, *lenp, "0x%04x\n", *(unsigned int *) table->data);

This adds a really hard to read overlong long line.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>