Date:   Thu, 27 Aug 2020 21:12:06 -0700
From:   Joe Perches <joe@...ches.com>
To:     Kees Cook <keescook@...omium.org>,
        Denis Efremov <efremov@...ux.com>
Cc:     Julia Lawall <julia.lawall@...ia.fr>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        linux-kernel@...r.kernel.org, cocci <cocci@...teme.lip6.fr>,
        Alex Dewar <alex.dewar90@...il.com>
Subject: Re: [Cocci] [PATCH] usb: atm: don't use snprintf() for sysfs attrs

On Thu, 2020-08-27 at 15:45 -0700, Joe Perches wrote:
> On Thu, 2020-08-27 at 15:20 -0700, Kees Cook wrote:
> > On Fri, Aug 28, 2020 at 12:01:34AM +0300, Denis Efremov wrote:
> > > Just FYI, I've send an addition to the device_attr_show.cocci script[1] to turn
> > > simple cases of snprintf (e.g. "%i") to sprintf. Looks like many developers would
> > > like it more than changing snprintf to scnprintf. As for me, I don't like the idea
> > > of automated altering of the original logic from bounded snprint to unbouded one
> > > with sprintf.
> > 
> > Agreed. This just makes me cringe. If the API design declares that when
> > a show() callback starts, buf has been allocated with PAGE_SIZE bytes,
> > then that's how the logic should proceed, and it should be using
> > scnprintf...
> > 
> > show(...) {
> > 	size_t remaining = PAGE_SIZE;
> > 
> > 	...
> > 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> > 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> > 	remaining -= scnprintf(buf, remaining, "fmt", var args ...);
> > 
> > 	return PAGE_SIZE - remaining;
> > }
> 
> It seems likely that coccinelle could do those transform
> with any of sprintf/snprintf/scnprint too.
> 
> Though my bikeshed would use a single function and have
> that function know the maximum output size

Perhaps something like the below with a sample conversion
that uses single and multiple sysfs_emit uses.

I believe coccinelle can _mostly_ automated this.

---
 fs/sysfs/file.c       | 30 ++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  8 ++++++++
 kernel/power/main.c   | 49 ++++++++++++++++++++++++++-----------------------
 3 files changed, 64 insertions(+), 23 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index eb6897ab78e7..c0ff3ba8e373 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -707,3 +707,33 @@ int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(sysfs_change_owner);
+
+/**
+ *	sysfs_emit - scnprintf equivalent, aware of PAGE_SIZE buffer.
+ *	@buf:	start of PAGE_SIZE buffer.
+ *	@pos:	current position in buffer
+ *              (pos - buf) must always be < PAGE_SIZE
+ *	@fmt:	format
+ *	@...:	arguments to format
+ *
+ *
+ * Returns number of characters written at pos.
+ */
+int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
+{
+	int len;
+	va_list args;
+
+	WARN(pos < buf, "pos < buf\n");
+	WARN(pos - buf >= PAGE_SIZE, "pos >= PAGE_SIZE (%tu > %lu)\n",
+	     pos - buf, PAGE_SIZE);
+	if (pos < buf || pos - buf >= PAGE_SIZE)
+		return 0;
+
+	va_start(args, fmt);
+	len = vscnprintf(pos, PAGE_SIZE - (pos - buf), fmt, args);
+	va_end(args);
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(sysfs_emit);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 34e84122f635..5a21d3d30016 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -329,6 +329,8 @@ int sysfs_groups_change_owner(struct kobject *kobj,
 int sysfs_group_change_owner(struct kobject *kobj,
 			     const struct attribute_group *groups, kuid_t kuid,
 			     kgid_t kgid);
+__printf(3, 4)
+int sysfs_emit(char *buf, char *pos, const char *fmt, ...);
 
 #else /* CONFIG_SYSFS */
 
@@ -576,6 +578,12 @@ static inline int sysfs_group_change_owner(struct kobject *kobj,
 	return 0;
 }
 
+__printf(3, 4)
+static inline int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
+{
+	return 0;
+}
+
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 40f86ec4ab30..f3fb9f255234 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -100,7 +100,7 @@ int pm_async_enabled = 1;
 static ssize_t pm_async_show(struct kobject *kobj, struct kobj_attribute *attr,
 			     char *buf)
 {
-	return sprintf(buf, "%d\n", pm_async_enabled);
+	return sysfs_emit(buf, buf, "%d\n", pm_async_enabled);
 }
 
 static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -124,7 +124,7 @@ power_attr(pm_async);
 static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
 			      char *buf)
 {
-	char *s = buf;
+	char *pos = buf;
 	suspend_state_t i;
 
 	for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
@@ -132,16 +132,16 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
 			const char *label = mem_sleep_states[i];
 
 			if (mem_sleep_current == i)
-				s += sprintf(s, "[%s] ", label);
+				pos += sysfs_emit(buf, pos, "[%s] ", label);
 			else
-				s += sprintf(s, "%s ", label);
+				pos += sysfs_emit(buf, pos, "%s ", label);
 		}
 
 	/* Convert the last space to a newline if needed. */
-	if (s != buf)
-		*(s-1) = '\n';
+	if (pos != buf)
+		*(pos - 1) = '\n';
 
-	return (s - buf);
+	return pos - buf;
 }
 
 static suspend_state_t decode_suspend_state(const char *buf, size_t n)
@@ -202,7 +202,7 @@ bool sync_on_suspend_enabled = !IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC);
 static ssize_t sync_on_suspend_show(struct kobject *kobj,
 				   struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", sync_on_suspend_enabled);
+	return sysfs_emit(buf, buf, "%d\n", sync_on_suspend_enabled);
 }
 
 static ssize_t sync_on_suspend_store(struct kobject *kobj,
@@ -336,7 +336,7 @@ static ssize_t last_failed_dev_show(struct kobject *kobj,
 	index %= REC_FAILED_NUM;
 	last_failed_dev = suspend_stats.failed_devs[index];
 
-	return sprintf(buf, "%s\n", last_failed_dev);
+	return sysfs_emit(buf, buf, "%s\n", last_failed_dev);
 }
 static struct kobj_attribute last_failed_dev = __ATTR_RO(last_failed_dev);
 
@@ -350,7 +350,7 @@ static ssize_t last_failed_errno_show(struct kobject *kobj,
 	index %= REC_FAILED_NUM;
 	last_failed_errno = suspend_stats.errno[index];
 
-	return sprintf(buf, "%d\n", last_failed_errno);
+	return sysfs_emit(buf, buf, "%d\n", last_failed_errno);
 }
 static struct kobj_attribute last_failed_errno = __ATTR_RO(last_failed_errno);
 
@@ -366,7 +366,7 @@ static ssize_t last_failed_step_show(struct kobject *kobj,
 	step = suspend_stats.failed_steps[index];
 	last_failed_step = suspend_step_name(step);
 
-	return sprintf(buf, "%s\n", last_failed_step);
+	return sysfs_emit(buf, buf, "%s\n", last_failed_step);
 }
 static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
 
@@ -474,7 +474,7 @@ bool pm_print_times_enabled;
 static ssize_t pm_print_times_show(struct kobject *kobj,
 				   struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", pm_print_times_enabled);
+	return sysfs_emit(buf, buf, "%d\n", pm_print_times_enabled);
 }
 
 static ssize_t pm_print_times_store(struct kobject *kobj,
@@ -504,7 +504,9 @@ static ssize_t pm_wakeup_irq_show(struct kobject *kobj,
 					struct kobj_attribute *attr,
 					char *buf)
 {
-	return pm_wakeup_irq ? sprintf(buf, "%u\n", pm_wakeup_irq) : -ENODATA;
+	if (!pm_wakeup_irq)
+		return -ENODATA;
+	return sysfs_emit(buf, buf, "%u\n", pm_wakeup_irq);
 }
 
 power_attr_ro(pm_wakeup_irq);
@@ -514,7 +516,7 @@ bool pm_debug_messages_on __read_mostly;
 static ssize_t pm_debug_messages_show(struct kobject *kobj,
 				      struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", pm_debug_messages_on);
+	return sysfs_emit(buf, buf, "%d\n", pm_debug_messages_on);
 }
 
 static ssize_t pm_debug_messages_store(struct kobject *kobj,
@@ -704,8 +706,9 @@ static ssize_t wakeup_count_show(struct kobject *kobj,
 {
 	unsigned int val;
 
-	return pm_get_wakeup_count(&val, true) ?
-		sprintf(buf, "%u\n", val) : -EINTR;
+	if (!pm_get_wakeup_count(&val, true))
+		return -EINTR;
+	return sysfs_emit(buf, buf, "%u\n", val);
 }
 
 static ssize_t wakeup_count_store(struct kobject *kobj,
@@ -747,17 +750,17 @@ static ssize_t autosleep_show(struct kobject *kobj,
 	suspend_state_t state = pm_autosleep_state();
 
 	if (state == PM_SUSPEND_ON)
-		return sprintf(buf, "off\n");
+		return sysfs_emit(buf, buf, "off\n");
 
 #ifdef CONFIG_SUSPEND
 	if (state < PM_SUSPEND_MAX)
-		return sprintf(buf, "%s\n", pm_states[state] ?
-					pm_states[state] : "error");
+		return sysfs_emit(buf, buf, "%s\n",
+				  pm_states[state] ?: "error");
 #endif
 #ifdef CONFIG_HIBERNATION
-	return sprintf(buf, "disk\n");
+	return sysfs_emit(buf, buf, "disk\n");
 #else
-	return sprintf(buf, "error");
+	return sysfs_emit(buf, buf, "error\n");
 #endif
 }
 
@@ -826,7 +829,7 @@ int pm_trace_enabled;
 static ssize_t pm_trace_show(struct kobject *kobj, struct kobj_attribute *attr,
 			     char *buf)
 {
-	return sprintf(buf, "%d\n", pm_trace_enabled);
+	return sysfs_emit(buf, buf, "%d\n", pm_trace_enabled);
 }
 
 static ssize_t
@@ -863,7 +866,7 @@ power_attr_ro(pm_trace_dev_match);
 static ssize_t pm_freeze_timeout_show(struct kobject *kobj,
 				      struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%u\n", freeze_timeout_msecs);
+	return sysfs_emit(buf, buf, "%u\n", freeze_timeout_msecs);
 }
 
 static ssize_t pm_freeze_timeout_store(struct kobject *kobj,

Powered by blists - more mailing lists