[RFC][CFLART] ipmi procfs bogosity

[RFC][CFLART] ipmi procfs bogosity

[viro]

drivers/char/ipmi/ipmi_poweroff.c::proc_write_chassctrl()
	a) does sscanf on userland pointer
	b) does sscanf on array that is not guaranteed to have NUL in it
	c) interprets input in incredibly cretinous way:
if strings doesn't start with a decimal number => as if it was "0".
if it starts with decimal number equal to 0 (e.g. is "-0000splat") - as if
it was "0".
if it starts with decimal number equal to 2 (e.g. is "00002FOAD") - as if
it was "2".
otherwise - -EINVAL.
	In any case that doesn't end up with -EINVAL, pretend that entire
buffer had been written.

(a) and (b) are immediate bugs; (c) is a valid reason for immediate severe
LARTing of the pervert who had done _that_ in a user-visible API.

Note that API _is_ user-visible, so we can't blindly change it - not without
checking WTF do its users actually write to /proc/ipmi/poweroff_control.

Could somebody comment on the actual uses of that FPOS?  My preference would
be to remove the damn thing completely - it's too ugly to live.

Re: [RFC][CFLART] ipmi procfs bogosity

[Corey Minyard]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

Indeed, this function is badly written.  In rewriting, I couldn't find a
nice function for reading integers from userspace, and the proc_dointvec
stuff didn't seem terribly suitable.  So I wrote my own general
function, which I can move someplace else if someone likes.  Patch is
attached.  It should not affect correct usage of this file.

Thank you for pointing this out.

Andrew, can you include this?  Matt, can you test?

-Corey

On Thu, 2005-09-01 at 07:43 +0100, viro@ZenIV.linux.org.uk wrote:
> drivers/char/ipmi/ipmi_poweroff.c::proc_write_chassctrl()
> 	a) does sscanf on userland pointer
> 	b) does sscanf on array that is not guaranteed to have NUL in it
> 	c) interprets input in incredibly cretinous way:
> if strings doesn't start with a decimal number => as if it was "0".
> if it starts with decimal number equal to 0 (e.g. is "-0000splat") - as if
> it was "0".
> if it starts with decimal number equal to 2 (e.g. is "00002FOAD") - as if
> it was "2".
> otherwise - -EINVAL.
> 	In any case that doesn't end up with -EINVAL, pretend that entire
> buffer had been written.
> 
> (a) and (b) are immediate bugs; (c) is a valid reason for immediate severe
> LARTing of the pervert who had done _that_ in a user-visible API.
> 
> Note that API _is_ user-visible, so we can't blindly change it - not without
> checking WTF do its users actually write to /proc/ipmi/poweroff_control.
> 
> Could somebody comment on the actual uses of that FPOS?  My preference would
> be to remove the damn thing completely - it's too ugly to live.

[-- Attachment #2: ipmi-poweroff-fix-chassis-ctrl.patch --]
[-- Type: text/x-patch, Size: 5572 bytes --]

The IPMI power control function proc_write_chassctrl was badly
written, it directly used userspace pointers, it assumed that
strings were NULL terminated, and it used the evil sscanf function.
This adds a new function to read integers from userspace and
uses this function to get the integer in question.

Signed-off-by: Corey Minyard <minyard@acm.org>

Index: linux-2.6.13/drivers/char/ipmi/ipmi_poweroff.c
===================================================================
--- linux-2.6.13.orig/drivers/char/ipmi/ipmi_poweroff.c
+++ linux-2.6.13/drivers/char/ipmi/ipmi_poweroff.c
@@ -40,6 +40,8 @@
 #include <linux/kdev_t.h>
 #include <linux/ipmi.h>
 #include <linux/ipmi_smi.h>
+#include <linux/ctype.h>
+#include <asm/uaccess.h>
 
 #define PFX "IPMI poweroff: "
 
@@ -545,27 +547,179 @@ static int proc_read_chassctrl(char *pag
 			poweroff_control);
 }
 
+/*
+ * Convert a character to an integer, returning -1 if the character is
+ * not a valid digit for "base".
+ */
+static int convdigit(char ch, int base)
+{
+	int v;
+	if ((ch >= '0') && (ch <= '9'))
+		v = ch - '0';
+	else if ((ch >= 'A') && (ch <= 'Z'))
+		v = ch - 'A' + 10;
+	else if ((ch >= 'a') && (ch <= 'z'))
+		v = ch - 'a' + 10;
+	else
+		return -1;
+	if (v >= base)
+		return -1;
+	return v;
+}
+
+/*
+ * Get a unsigned long from userspace, much like strtoul, but getting
+ * the data from userspace.  This is a little complicated to use, but
+ * is quite flexible.
+ *
+ * @buffer - is a pointer to the start of the user buffer.  Basically,
+ * this will read from buffer+*pos up to buffer+count-1.
+ * @pos - points to the start offset in buffer to start reading at.
+ * pos will be updated to the current location after the scan upon return.
+ * May be NULL, where it is assumed to be zero.
+ * @count - the total buffer length.
+ * @fbase - forces the base.  If this value is zero 0, the base will be
+ * hex if the string starts with 0x, octal if it starts with 0, and
+ * decimal otherwise.
+ * @stop_at_inval - a flag that, if true, causes the function to
+ * return if it already has a valid number and has reached an invalid
+ * character.  "pos" will be set to the offset of the invalid
+ * character upon return.  If false, the entire buffer is expected to
+ * be a single integer value surrounded by optional space characters.
+ *
+ * Unlike strtoul, the value is returned in the "val" parameter.  The
+ * return value of this function is a errno if negative or the number
+ * of characters processed if positive.
+ */
+static ssize_t user_strtoul(const char __user *buffer, loff_t *ppos,
+			    size_t count, int fbase, int stop_at_inval,
+			    unsigned long *val)
+{
+	unsigned long newval = 0;
+	char          buf[20];
+	unsigned int  rc;
+	unsigned int  i;
+	enum { SC_ST, SC_ST2, SC_AFTPRE, SC_DAT, SC_END } state = SC_ST;
+	int           base = fbase;
+	loff_t        start;
+	loff_t        pos;
+
+	if (ppos)
+		pos = *ppos;
+	else
+		pos = 0;
+	start = pos;
+	count -= pos;
+	if (base == 0)
+		base = 10;
+	while (count > 0) {
+		rc = min(count, (size_t) sizeof(buf));
+		if (copy_from_user(buf, buffer + pos, rc))
+			return -EFAULT;
+		for (i = 0; i < rc; i++) {
+			char ch = buf[i];
+			int  chval;
+
+			switch(state) {
+			case SC_ST:
+				if (isspace(ch))
+					break;
+				if (ch == '0') {
+					if (fbase == 0)
+						base = 8;
+					state = SC_ST2;
+					break;
+				}
+				goto aftpre;
+
+			case SC_ST2:
+				if (ch == 'x') {
+					if (fbase == 0)
+						base = 16;
+					else if (fbase != 16)
+						return -EINVAL;
+					state = SC_AFTPRE;
+					break;
+				}
+				goto aftpre;
+
+			case SC_AFTPRE:
+			aftpre:
+				chval = convdigit(ch, base);
+				if (chval == -1)
+					return -EINVAL;
+				newval = (newval * base) + chval;
+				state = SC_DAT;
+				break;
+
+			case SC_DAT:
+				if (isspace(ch)) {
+					if (stop_at_inval) {
+						pos += i;
+						goto out;
+					}
+					state = SC_END;
+				} else {
+					unsigned long oldval = newval;
+					chval = convdigit(ch, base);
+					if (chval == -1) {
+						if (stop_at_inval) {
+							pos += i;
+							goto out;
+						}
+						return -EINVAL;
+					}
+					newval *= base;
+					if (newval < oldval) /* overflow */
+						return -EINVAL;
+					oldval = newval;
+					newval += chval;
+					if (newval < oldval) /* overflow */
+						return -EINVAL;
+				}
+				break;
+
+			case SC_END:
+				if (!isspace(ch))
+					return -EINVAL;
+				break;
+			}
+		}
+
+		count -= rc;
+		pos += rc;
+	}
+
+ out:
+	if (ppos)
+		*ppos = pos;
+	*val = newval;
+	return pos - start;
+}
+
 /* process property writes from proc */
-static int proc_write_chassctrl(struct file *file, const char *buffer,
+static int proc_write_chassctrl(struct file *file, const char __user *buffer,
 			        unsigned long count, void *data)
 {
-	int          rv = count;
-	unsigned int newval = 0;
+	unsigned long newval;
+	ssize_t       rv;
 
-	sscanf(buffer, "%d", &newval);
+	rv = user_strtoul(buffer, NULL, count, 0, 0, &newval);
+	if (rv < 0)
+		return rv;
 	switch (newval) {
-		case IPMI_CHASSIS_POWER_CYCLE:
-			printk(KERN_INFO PFX "power cycle is now enabled\n");
-			poweroff_control = newval;
-			break;
-
-		case IPMI_CHASSIS_POWER_DOWN:
-			poweroff_control = IPMI_CHASSIS_POWER_DOWN;
-			break;
-
-		default:
-			rv = -EINVAL;
-			break;
+	case IPMI_CHASSIS_POWER_CYCLE:
+		printk(KERN_INFO PFX "power cycle is now enabled\n");
+		poweroff_control = newval;
+		break;
+
+	case IPMI_CHASSIS_POWER_DOWN:
+		poweroff_control = IPMI_CHASSIS_POWER_DOWN;
+		break;
+
+	default:
+		rv = -EINVAL;
+		break;
 	}
 
 	return rv;

Re: [RFC][CFLART] ipmi procfs bogosity

[viro]

On Thu, Sep 01, 2005 at 11:41:42AM -0500, Corey Minyard wrote:
> Indeed, this function is badly written.  In rewriting, I couldn't find a
> nice function for reading integers from userspace, and the proc_dointvec
> stuff didn't seem terribly suitable.  So I wrote my own general
> function, which I can move someplace else if someone likes.  Patch is
> attached.  It should not affect correct usage of this file.

Eeeek...  Much, _much_ simpler approach would be to have

	char buf[10];
	if (count > 9)
		return -EINVAL;
	if (copy_from_user(buf, buffer, count))
		return -EFAULT;
	buf[count] = '\0';
	/* use sscanf() or anything normal */

Would that change behaviour in any cases you care about?

Re: [RFC][CFLART] ipmi procfs bogosity

[Corey Minyard]

viro@ZenIV.linux.org.uk wrote:

>On Thu, Sep 01, 2005 at 11:41:42AM -0500, Corey Minyard wrote:
>  
>
>>Indeed, this function is badly written.  In rewriting, I couldn't find a
>>nice function for reading integers from userspace, and the proc_dointvec
>>stuff didn't seem terribly suitable.  So I wrote my own general
>>function, which I can move someplace else if someone likes.  Patch is
>>attached.  It should not affect correct usage of this file.
>>    
>>
>
>Eeeek...  Much, _much_ simpler approach would be to have
>
>	char buf[10];
>	if (count > 9)
>		return -EINVAL;
>	if (copy_from_user(buf, buffer, count))
>		return -EFAULT;
>	buf[count] = '\0';
>	/* use sscanf() or anything normal */
>
>Would that change behaviour in any cases you care about?
>  
>
Because then, for a general solution that avoids integer
perversion, you need something like:

    char         buf[10];
    char         *end;

    if (count > (sizeof(buf) - 1))
        return -EINVAL;
    if (copy_from_user(buf, buffer, count))
        return -EFAULT;
    buf[count] = '\0';
    newval = simple_strtoul(buf, &end, 0);
    if (buf == end)
        /* Empty string or first char not a number */
        return -EINVAL;
    if (*end && ! isspace(*end))
        /* Bogus number. */
        return -EINVAL;


To me, It's a lot nicer to do:

    rv = user_strtoul(....);
    if (rv < 0)
        return rv;

Plus the scanning function I wrote handles arbitrary leading and 
trailing space, etc.  Not a big deal, but a little nicer.

-Corey

Re: [RFC][CFLART] ipmi procfs bogosity

[Alexey Dobriyan]

On Thu, Sep 01, 2005 at 03:00:44PM -0500, Corey Minyard wrote:
> To me, It's a lot nicer to do:
> 
>    rv = user_strtoul(....);
>    if (rv < 0)
>        return rv;
> 
> Plus the scanning function I wrote handles arbitrary leading and 
> trailing space, etc.  Not a big deal, but a little nicer.

You can say from the beggining that

	echo -n "    2   " >/proc/FUBAR
	
is illegal and don't add bloat to kernel.

Re: [RFC][CFLART] ipmi procfs bogosity

[Ingo Oeser]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

Hi,

On Thursday 01 September 2005 22:30, Alexey Dobriyan wrote:
> On Thu, Sep 01, 2005 at 03:00:44PM -0500, Corey Minyard wrote:
> > Plus the scanning function I wrote handles arbitrary leading and 
> > trailing space, etc.  Not a big deal, but a little nicer.
> 
> You can say from the beggining that
> 
> 	echo -n "    2   " >/proc/FUBAR
> 	
> is illegal and don't add bloat to kernel.

No, user interfaces should be robust.
Just remember the mantra "Be liberal in what you accept and
conservative in what you send."

I would suggest adding sth. like Coreys user_strtoul() to lib/string.c
which would reduce bloat and security threats for the kernel.


Regards

Ingo Oeser


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC][CFLART] ipmi procfs bogosity

[Alexey Dobriyan]

On Mon, Sep 05, 2005 at 12:51:32PM +0200, Ingo Oeser wrote:
> I would suggest adding sth. like Coreys user_strtoul()

strtoul_from_user

> to lib/string.c

lib/vsprintf.c

> which would reduce bloat and security threats for the kernel.

Re: [RFC][CFLART] ipmi procfs bogosity

[Andrew Morton]

Corey Minyard <minyard@acm.org> wrote:
>
> Indeed, this function is badly written.  In rewriting, I couldn't find a
>  nice function for reading integers from userspace, and the proc_dointvec
>  stuff didn't seem terribly suitable.

We write numbers into profs files all the time.  Is there something
different about the IPMI requirement which makes the approach used by, say,
dirty_writeback_centisecs_handler() inappropriate?

Re: [RFC][CFLART] ipmi procfs bogosity

[Corey Minyard]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

Andrew Morton wrote:

>Corey Minyard <minyard@acm.org> wrote:
>  
>
>>Indeed, this function is badly written.  In rewriting, I couldn't find a
>> nice function for reading integers from userspace, and the proc_dointvec
>> stuff didn't seem terribly suitable.
>>    
>>
>
>We write numbers into profs files all the time.  Is there something
>different about the IPMI requirement which makes the approach used by, say,
>dirty_writeback_centisecs_handler() inappropriate?
>  
>
Ok, that's probably better, and this probably belongs in 
/proc/sys/dev/ipmi.  This is new enough that it doesn't matter, I don't 
think any one is using it yet.

Patch is attached.

-Corey

[-- Attachment #2: ipmi-poweroff-fix-chassis-ctrl.patch --]
[-- Type: unknown/unknown, Size: 8332 bytes --]