[PATCH RFC 0/2] simple sysfs wrappers for single values

[PATCH RFC 0/2] simple sysfs wrappers for single values

[Alex Dewar]

Hi all,

I've noticed there seems to have been a fair amount of discussion around
the subject of possible helper methods for use in the context of sysfs
show methods (which I haven't had a chance to go through in detail yet
-- sorry!), so I thought I'd send out a couple of patches I've been
working on for this, in case it's of any interest to anyone.

My idea was to have a few simple wrappers for returning single values
via sysfs, as in theory that's how sysfs should be being used. This
isn't going to be usable for more complicated cases, but at least by
doing this we should be able to make it easier to direct the attention
of code checkers (either automated or the flesh-and-blood kind) towards
the potentially more problematic cases. Hopefully we should be able to
convert over many of the more trivial cases to these helpers using
Coccinelle.

For the number helper (sysfs_sprinti), I opted to go for a macro which
can handle short, int, ULL etc., though equally we could go for
differently named inline functions instead. Either way, I think
enforcing the type of input arguments and not allowing users to pass
format strings is the way to go. Even for e.g. "sprintf(buf, "%llu\n",
my_number)", there is the possibility that my_number could be of type
int and so mishandle negative values. Let's make it easy for maintainers
to review at a glance, without having to remember what type my_number
was declared as.

With the string helpers we can make sure that we aren't overflowing the
destination buffer, but I've also put in a check so that we don't read
beyond the end of the source buffer either, in the case that it's a
fixed-size array. So if we do have an automated changeover with
Coccinelle then we should make things safer too :-)

Anyway, I'm off to bed now but I'll check for messages in the morning.
Any feedback gratefully received!

Best,
Alex

[PATCH RFC 1/2] sysfs: add helpers for safely showing simple strings

[Alex Dewar]

Add a helper, sysfs_strscpy(), which simply copies a string and
appends a newline and NUL char to the end, making sure not to overflow
the destination buffer, which MUST be PAGE_SIZE bytes (which is true for
buffers in this context). It includes a compile time check for the
specified destination buffer size.

Also add a helper macro, sysfs_strcpy(), which calls sysfs_strscpy() with
count == PAGE_SIZE, for use with regular NUL-terminated strings. If the
src buffer is a fixed-size array, guarantee that we don't copy beyond its
end by only copying a maximum of sizeof(src) bytes.

Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
 fs/sysfs/file.c       | 14 ++++++++++++++
 include/linux/sysfs.h | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index eb6897ab78e7..2a60e5c6392d 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -707,3 +707,17 @@ int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(sysfs_change_owner);
+
+ssize_t __sysfs_strscpy(char *dest, const char *src, size_t count)
+{
+	ssize_t written;
+
+	if (count > PAGE_SIZE)
+		return -EINVAL;
+
+	written = strscpy(dest, src, count - 1);
+	dest[written++] = '\n';
+	dest[written] = '\0';
+	return written;
+}
+EXPORT_SYMBOL_GPL(__sysfs_strscpy);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 34e84122f635..26e7d9f69dfd 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -162,6 +162,41 @@ static const struct attribute_group _name##_group = {		\
 };								\
 __ATTRIBUTE_GROUPS(_name)
 
+/**
+ *	sysfs_strscpy - return a string from a show method with terminating
+ *	newline to a maximum of count bytes
+ *	@dest: destination buffer
+ *	@src: string to be emitted
+ *	@count: maximum number of bytes to be written to dest
+ */
+#define sysfs_strscpy(dest, src, count)                                       \
+({                                                                            \
+	BUILD_BUG_ON(__builtin_constant_p(count) && (count) > PAGE_SIZE);      \
+	__sysfs_strscpy((dest), (src), (count));                               \
+})
+
+ssize_t __sysfs_strscpy(char *dest, const char *src, size_t count);
+
+/**
+ *	sysfs_strcpy - return a string from a show method with terminating
+ *	newline
+ *	@dest: destination buffer
+ *	@src: string to be emitted
+ *
+ *	This method will only write a maximum of PAGE_SIZE bytes to dest,
+ *	ensuring that the output buffer is not overflown. If src is a
+ *	fixed-size array, a maximum of sizeof(src) bytes will be copied,
+ *	ensuring that memory is not read beyond the end of the array.
+ */
+#define sysfs_strcpy(dest, src)                                      \
+sysfs_strscpy((dest), (src),                                         \
+		__builtin_choose_expr(                                \
+			__same_type((src), &(src)[0]),                \
+			PAGE_SIZE,                                    \
+			min(sizeof(src) + 2, PAGE_SIZE)               \
+		)                                                     \
+)
+
 struct file;
 struct vm_area_struct;
 
-- 
2.28.0

[PATCH RFC 2/2] sysfs: add helper macro for showing simple integer values

[Alex Dewar]

sysfs attributes are supposed to be only single values, which are
printed into a buffer of PAGE_SIZE. Accordingly, for many simple
attributes, sprintf() can be used like so:
	static ssize_t my_show(..., char *buf)
	{
		...
		return sprintf("%d\n", my_integer);
	}

The problem is that whilst this use of sprintf() is memory safe, other
cases where e.g. a possibly unterminated string is passed as input, are
not and so use of sprintf() here might make it more difficult to
identify these problematic cases.

Define a macro, sysfs_sprinti(), which outputs the value of a single
integer to a buffer (with terminating "\n\0") and returns the size written.
This way, we can convert over the some of the trivially correct users of
sprintf() and decrease its usage in the kernel source tree.

Another advantage of this approach is that we can now statically check
the type of the integer so that e.g. an unsigned long long will be
formatted as %llu. This will fix cases where the wrong format string has
been passed to sprintf().

Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
 include/linux/sysfs.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 26e7d9f69dfd..763316788153 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -197,6 +197,37 @@ sysfs_strscpy((dest), (src),                                         \
 		)                                                     \
 )
 
+/**
+ *	sysfs_sprinti - emit an integer-type value from a sysfs show method
+ *	@buf: destination buffer
+ *	@x: the variable whose value is to be shown
+ *
+ *	The appropriate format is passed to sprintf() according to the type of
+ *	x, preventing accidental misuse of format strings.
+ */
+#define sysfs_sprinti(buf, x)                                                               \
+({                                                                                          \
+	BUILD_BUG_ON(!__builtin_types_compatible_p(typeof(x), unsigned int) &&               \
+			!__builtin_types_compatible_p(typeof(x), unsigned long) &&           \
+			!__builtin_types_compatible_p(typeof(x), unsigned long long) &&      \
+			!__builtin_types_compatible_p(typeof(x), int) &&                     \
+			!__builtin_types_compatible_p(typeof(x), short) &&                   \
+			!__builtin_types_compatible_p(typeof(x), unsigned short));           \
+	__builtin_choose_expr(                                                               \
+		__builtin_types_compatible_p(typeof(x), unsigned int),                       \
+		sprintf(buf, "%u\n", (unsigned int)(x)),                                     \
+		__builtin_choose_expr(                                                       \
+			__builtin_types_compatible_p(typeof(x), unsigned long),              \
+			sprintf(buf, "%lu\n", (unsigned long)(x)),                           \
+			__builtin_choose_expr(                                               \
+				__builtin_types_compatible_p(typeof(x), unsigned long long), \
+				sprintf(buf, "%llu\n", (unsigned long long)(x)),             \
+				sprintf(buf, "%d\n", (int)(x))                               \
+			)                                                                    \
+		)                                                                            \
+	);                                                                                   \
+})
+
 struct file;
 struct vm_area_struct;
 
-- 
2.28.0

Re: [PATCH RFC 2/2] sysfs: add helper macro for showing simple integer values

[Greg Kroah-Hartman]

On Sun, Aug 30, 2020 at 12:37:17AM +0100, Alex Dewar wrote:
> sysfs attributes are supposed to be only single values, which are
> printed into a buffer of PAGE_SIZE. Accordingly, for many simple
> attributes, sprintf() can be used like so:
> 	static ssize_t my_show(..., char *buf)
> 	{
> 		...
> 		return sprintf("%d\n", my_integer);
> 	}
> 
> The problem is that whilst this use of sprintf() is memory safe, other
> cases where e.g. a possibly unterminated string is passed as input, are
> not and so use of sprintf() here might make it more difficult to
> identify these problematic cases.
> 
> Define a macro, sysfs_sprinti(), which outputs the value of a single
> integer to a buffer (with terminating "\n\0") and returns the size written.
> This way, we can convert over the some of the trivially correct users of
> sprintf() and decrease its usage in the kernel source tree.
> 
> Another advantage of this approach is that we can now statically check
> the type of the integer so that e.g. an unsigned long long will be
> formatted as %llu. This will fix cases where the wrong format string has
> been passed to sprintf().
> 
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
>  include/linux/sysfs.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)

Did you try this out?  Don't you need to return the number of bytes
written?

I like Joe's patches better, this feels like more work...

thanks,

greg k-h

Re: [PATCH RFC 2/2] sysfs: add helper macro for showing simple integer values

[Alex Dewar]

On Sun, Aug 30, 2020 at 11:13:53AM +0200, Greg Kroah-Hartman wrote:
> On Sun, Aug 30, 2020 at 12:37:17AM +0100, Alex Dewar wrote:
> > sysfs attributes are supposed to be only single values, which are
> > printed into a buffer of PAGE_SIZE. Accordingly, for many simple
> > attributes, sprintf() can be used like so:
> > 	static ssize_t my_show(..., char *buf)
> > 	{
> > 		...
> > 		return sprintf("%d\n", my_integer);
> > 	}
> > 
> > The problem is that whilst this use of sprintf() is memory safe, other
> > cases where e.g. a possibly unterminated string is passed as input, are
> > not and so use of sprintf() here might make it more difficult to
> > identify these problematic cases.
> > 
> > Define a macro, sysfs_sprinti(), which outputs the value of a single
> > integer to a buffer (with terminating "\n\0") and returns the size written.
> > This way, we can convert over the some of the trivially correct users of
> > sprintf() and decrease its usage in the kernel source tree.
> > 
> > Another advantage of this approach is that we can now statically check
> > the type of the integer so that e.g. an unsigned long long will be
> > formatted as %llu. This will fix cases where the wrong format string has
> > been passed to sprintf().
> > 
> > Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> > ---
> >  include/linux/sysfs.h | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> 
> Did you try this out?  Don't you need to return the number of bytes
> written?

I tried it out, but maybe not thoroughly enough ;-)

> 
> I like Joe's patches better, this feels like more work...

Fair enough. As Joe's pointed out, even for single numbers the
formatting is sometimes more complicated, so his approach does seem
best. Thanks for looking though :-)

> 
> thanks,
> 
> greg k-h

Re: [PATCH RFC 0/2] simple sysfs wrappers for single values

[Joe Perches]

On Sun, 2020-08-30 at 00:37 +0100, Alex Dewar wrote:
> Hi all,
> 
> I've noticed there seems to have been a fair amount of discussion around
> the subject of possible helper methods for use in the context of sysfs
> show methods (which I haven't had a chance to go through in detail yet
> -- sorry!), so I thought I'd send out a couple of patches I've been
> working on for this, in case it's of any interest to anyone.

If you really want to do this, I suggest you get use
wrappers like sysfs_emit_string, sysfs_emit_int, sysfs_emit_u64
though I don't see _that_ much value.

Re: [PATCH RFC 0/2] simple sysfs wrappers for single values

[Joe Perches]

On Sat, 2020-08-29 at 17:28 -0700, Joe Perches wrote:
> On Sun, 2020-08-30 at 00:37 +0100, Alex Dewar wrote:
> > Hi all,
> > 
> > I've noticed there seems to have been a fair amount of discussion around
> > the subject of possible helper methods for use in the context of sysfs
> > show methods (which I haven't had a chance to go through in detail yet
> > -- sorry!), so I thought I'd send out a couple of patches I've been
> > working on for this, in case it's of any interest to anyone.
> 
> If you really want to do this, I suggest you get use
> wrappers like sysfs_emit_string, sysfs_emit_int, sysfs_emit_u64
> though I don't see _that_ much value.

Just fyi:

the treewide converted sysfs_emit uses
end up with these integer outputs:

$ git grep -P -oh 'sysfs_emit\(buf, "%\d*[luixd]*\\n"' | \
  sort | uniq -c | sort -rn
   1482 sysfs_emit(buf, "%d\n"
    549 sysfs_emit(buf, "%u\n"
    118 sysfs_emit(buf, "%ld\n"
    100 sysfs_emit(buf, "%lu\n"
     78 sysfs_emit(buf, "%llu\n"
     62 sysfs_emit(buf, "%i\n"
     47 sysfs_emit(buf, "%x\n"
     24 sysfs_emit(buf, "%lld\n"
     12 sysfs_emit(buf, "%llx\n"
     12 sysfs_emit(buf, "%08x\n"
     12 sysfs_emit(buf, "%02x\n"
     10 sysfs_emit(buf, "%016llx\n"
      8 sysfs_emit(buf, "%04x\n"
      6 sysfs_emit(buf, "%lx\n"
      5 sysfs_emit(buf, "%02d\n"
      4 sysfs_emit(buf, "%04d\n"
      2 sysfs_emit(buf, "%08lx\n"
      1 sysfs_emit(buf, "%li\n"
      1 sysfs_emit(buf, "%4x\n"
      1 sysfs_emit(buf, "%0x\n"
      1 sysfs_emit(buf, "%06x\n"
      1 sysfs_emit(buf, "%03x\n"
      1 sysfs_emit(buf, "%01x\n"
> 

Re: [PATCH RFC 0/2] simple sysfs wrappers for single values

[Alex Dewar]

On Sat, Aug 29, 2020 at 06:23:13PM -0700, Joe Perches wrote:
> On Sat, 2020-08-29 at 17:28 -0700, Joe Perches wrote:
> > On Sun, 2020-08-30 at 00:37 +0100, Alex Dewar wrote:
> > > Hi all,
> > > 
> > > I've noticed there seems to have been a fair amount of discussion around
> > > the subject of possible helper methods for use in the context of sysfs
> > > show methods (which I haven't had a chance to go through in detail yet
> > > -- sorry!), so I thought I'd send out a couple of patches I've been
> > > working on for this, in case it's of any interest to anyone.
> > 
> > If you really want to do this, I suggest you get use
> > wrappers like sysfs_emit_string, sysfs_emit_int, sysfs_emit_u64
> > though I don't see _that_ much value.
> 
> Just fyi:
> 
> the treewide converted sysfs_emit uses
> end up with these integer outputs:

Thanks for looking at the code. It does look like my approach was a bit
too simplistic!

> 
> $ git grep -P -oh 'sysfs_emit\(buf, "%\d*[luixd]*\\n"' | \
>   sort | uniq -c | sort -rn
>    1482 sysfs_emit(buf, "%d\n"
>     549 sysfs_emit(buf, "%u\n"
>     118 sysfs_emit(buf, "%ld\n"
>     100 sysfs_emit(buf, "%lu\n"
>      78 sysfs_emit(buf, "%llu\n"
>      62 sysfs_emit(buf, "%i\n"
>      47 sysfs_emit(buf, "%x\n"
>      24 sysfs_emit(buf, "%lld\n"
>      12 sysfs_emit(buf, "%llx\n"
>      12 sysfs_emit(buf, "%08x\n"
>      12 sysfs_emit(buf, "%02x\n"
>      10 sysfs_emit(buf, "%016llx\n"
>       8 sysfs_emit(buf, "%04x\n"
>       6 sysfs_emit(buf, "%lx\n"
>       5 sysfs_emit(buf, "%02d\n"
>       4 sysfs_emit(buf, "%04d\n"
>       2 sysfs_emit(buf, "%08lx\n"
>       1 sysfs_emit(buf, "%li\n"
>       1 sysfs_emit(buf, "%4x\n"
>       1 sysfs_emit(buf, "%0x\n"
>       1 sysfs_emit(buf, "%06x\n"
>       1 sysfs_emit(buf, "%03x\n"
>       1 sysfs_emit(buf, "%01x\n"
> > 
>