diff mbox series

[03/23] lib: uaccess: provide getline_from_user()

Message ID 20200904154547.3836-4-brgl@bgdev.pl (mailing list archive)
State Not Applicable, archived
Headers show
Series gpio: mockup: support dynamically created and removed chips | expand

Commit Message

Bartosz Golaszewski Sept. 4, 2020, 3:45 p.m. UTC
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Provide a uaccess helper that allows callers to copy a single line from
user memory. This is useful for debugfs write callbacks.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/uaccess.h |  3 +++
 lib/usercopy.c          | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

Comments

Andy Shevchenko Sept. 4, 2020, 4:35 p.m. UTC | #1
On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Provide a uaccess helper that allows callers to copy a single line from
> user memory. This is useful for debugfs write callbacks.

Doesn't mm/util.c provides us something like this?
strndup_user()?

> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/uaccess.h |  3 +++
>  lib/usercopy.c          | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 94b285411659..5aedd8ac5c31 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -333,6 +333,9 @@ long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
>  		long count);
>  long strnlen_user_nofault(const void __user *unsafe_addr, long count);
>  
> +ssize_t getline_from_user(char *dst, size_t dst_size,
> +			  const char __user *src, size_t src_size);
> +
>  /**
>   * get_kernel_nofault(): safely attempt to read from a location
>   * @val: read into this variable
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> index b26509f112f9..55aaaf93d847 100644
> --- a/lib/usercopy.c
> +++ b/lib/usercopy.c
> @@ -87,3 +87,40 @@ int check_zeroed_user(const void __user *from, size_t size)
>  	return -EFAULT;
>  }
>  EXPORT_SYMBOL(check_zeroed_user);
> +
> +/**
> + * getline_from_user - Copy a single line from user
> + * @dst: Where to copy the line to
> + * @dst_size: Size of the destination buffer
> + * @src: Where to copy the line from
> + * @src_size: Size of the source user buffer
> + *
> + * Copies a number of characters from given user buffer into the dst buffer.
> + * The number of bytes is limited to the lesser of the sizes of both buffers.
> + * If the copied string contains a newline, its first occurrence is replaced
> + * by a NULL byte in the destination buffer. Otherwise the function ensures
> + * the copied string is NULL-terminated.
> + *
> + * Returns the number of copied bytes or a negative error number on failure.
> + */
> +
> +ssize_t getline_from_user(char *dst, size_t dst_size,
> +			  const char __user *src, size_t src_size)
> +{
> +	size_t size = min_t(size_t, dst_size, src_size);
> +	char *c;
> +	int ret;
> +
> +	ret = copy_from_user(dst, src, size);
> +	if (ret)
> +		return -EFAULT;
> +
> +	dst[size - 1] = '\0';
> +
> +	c = strchrnul(dst, '\n');
> +	if (*c)
> +		*c = '\0';
> +
> +	return c - dst;
> +}
> +EXPORT_SYMBOL(getline_from_user);
> -- 
> 2.26.1
>
Bartosz Golaszewski Sept. 7, 2020, 10:02 a.m. UTC | #2
On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Provide a uaccess helper that allows callers to copy a single line from
> > user memory. This is useful for debugfs write callbacks.
>
> Doesn't mm/util.c provides us something like this?
> strndup_user()?
>

Yes, there's both strndup_user() as well as strncpy_from_user(). The
problem is that they rely on the strings being NULL-terminated. This
is not guaranteed for debugfs file_operations write callbacks. We need
some helper that takes the minimum of bytes provided by userspace and
the buffer size and figure out how many bytes to actually copy IMO.

Bart
Andy Shevchenko Sept. 7, 2020, 10:18 a.m. UTC | #3
On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > Doesn't mm/util.c provides us something like this?
> > strndup_user()?
> >
>
> Yes, there's both strndup_user() as well as strncpy_from_user(). The
> problem is that they rely on the strings being NULL-terminated. This
> is not guaranteed for debugfs file_operations write callbacks. We need
> some helper that takes the minimum of bytes provided by userspace and
> the buffer size and figure out how many bytes to actually copy IMO.

Wouldn't this [1] approach work?

[1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93
Bartosz Golaszewski Sept. 7, 2020, 10:28 a.m. UTC | #4
On Mon, Sep 7, 2020 at 12:19 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> > > Doesn't mm/util.c provides us something like this?
> > > strndup_user()?
> > >
> >
> > Yes, there's both strndup_user() as well as strncpy_from_user(). The
> > problem is that they rely on the strings being NULL-terminated. This
> > is not guaranteed for debugfs file_operations write callbacks. We need
> > some helper that takes the minimum of bytes provided by userspace and
> > the buffer size and figure out how many bytes to actually copy IMO.
>
> Wouldn't this [1] approach work?
>
> [1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93
>

Sure, but this is pretty much what I do in getline_from_user(). If
anything we should port mtrr_write() to using getline_from_user() once
it's available upstream, no?

Bart
Andy Shevchenko Sept. 7, 2020, 11:45 a.m. UTC | #5
On Mon, Sep 07, 2020 at 12:28:05PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 7, 2020 at 12:19 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > > On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > > > Doesn't mm/util.c provides us something like this?
> > > > strndup_user()?
> > > >
> > >
> > > Yes, there's both strndup_user() as well as strncpy_from_user(). The
> > > problem is that they rely on the strings being NULL-terminated. This
> > > is not guaranteed for debugfs file_operations write callbacks. We need
> > > some helper that takes the minimum of bytes provided by userspace and
> > > the buffer size and figure out how many bytes to actually copy IMO.
> >
> > Wouldn't this [1] approach work?
> >
> > [1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93
> >
> 
> Sure, but this is pretty much what I do in getline_from_user(). If
> anything we should port mtrr_write() to using getline_from_user() once
> it's available upstream, no?

But you may provide getline_from_user() as inline in the same header where
strncpy_from_user() is declared. It will be like 3 LOCs?
Bartosz Golaszewski Sept. 7, 2020, 11:57 a.m. UTC | #6
On Mon, Sep 7, 2020 at 1:45 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Sep 07, 2020 at 12:28:05PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 7, 2020 at 12:19 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski
> > > <bgolaszewski@baylibre.com> wrote:
> > > > On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > > > Doesn't mm/util.c provides us something like this?
> > > > > strndup_user()?
> > > > >
> > > >
> > > > Yes, there's both strndup_user() as well as strncpy_from_user(). The
> > > > problem is that they rely on the strings being NULL-terminated. This
> > > > is not guaranteed for debugfs file_operations write callbacks. We need
> > > > some helper that takes the minimum of bytes provided by userspace and
> > > > the buffer size and figure out how many bytes to actually copy IMO.
> > >
> > > Wouldn't this [1] approach work?
> > >
> > > [1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93
> > >
> >
> > Sure, but this is pretty much what I do in getline_from_user(). If
> > anything we should port mtrr_write() to using getline_from_user() once
> > it's available upstream, no?
>
> But you may provide getline_from_user() as inline in the same header where
> strncpy_from_user() is declared. It will be like 3 LOCs?
>

May be more than that. I'll see what I can do.

Bart
diff mbox series

Patch

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 94b285411659..5aedd8ac5c31 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -333,6 +333,9 @@  long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
 		long count);
 long strnlen_user_nofault(const void __user *unsafe_addr, long count);
 
+ssize_t getline_from_user(char *dst, size_t dst_size,
+			  const char __user *src, size_t src_size);
+
 /**
  * get_kernel_nofault(): safely attempt to read from a location
  * @val: read into this variable
diff --git a/lib/usercopy.c b/lib/usercopy.c
index b26509f112f9..55aaaf93d847 100644
--- a/lib/usercopy.c
+++ b/lib/usercopy.c
@@ -87,3 +87,40 @@  int check_zeroed_user(const void __user *from, size_t size)
 	return -EFAULT;
 }
 EXPORT_SYMBOL(check_zeroed_user);
+
+/**
+ * getline_from_user - Copy a single line from user
+ * @dst: Where to copy the line to
+ * @dst_size: Size of the destination buffer
+ * @src: Where to copy the line from
+ * @src_size: Size of the source user buffer
+ *
+ * Copies a number of characters from given user buffer into the dst buffer.
+ * The number of bytes is limited to the lesser of the sizes of both buffers.
+ * If the copied string contains a newline, its first occurrence is replaced
+ * by a NULL byte in the destination buffer. Otherwise the function ensures
+ * the copied string is NULL-terminated.
+ *
+ * Returns the number of copied bytes or a negative error number on failure.
+ */
+
+ssize_t getline_from_user(char *dst, size_t dst_size,
+			  const char __user *src, size_t src_size)
+{
+	size_t size = min_t(size_t, dst_size, src_size);
+	char *c;
+	int ret;
+
+	ret = copy_from_user(dst, src, size);
+	if (ret)
+		return -EFAULT;
+
+	dst[size - 1] = '\0';
+
+	c = strchrnul(dst, '\n');
+	if (*c)
+		*c = '\0';
+
+	return c - dst;
+}
+EXPORT_SYMBOL(getline_from_user);