SCSI: fix parsing of /proc/scsci/scsi file

Commit Message

Linus Torvalds July 25, 2023, 5:30 p.m. UTC

This is the simplified version of the fix proposed by Tony Battersby
for the horrid scsi /proc parsing code.

It doesn't make it prettier, it just makes it less buggy. That parsing
code hasn't been changed in git or BK times, so it's at least two
decades since anybody touched it, and judging by how nasty it is, it's
probably more than that.

This is v2 with the additional bug noted by Tony hopefully fixed.

             Linus

Comments

Tony Battersby July 25, 2023, 6:27 p.m. UTC | #1

On 7/25/23 13:30, Linus Torvalds wrote:
> This is the simplified version of the fix proposed by Tony Battersby
> for the horrid scsi /proc parsing code.
>
Something that I just thought of: the old parser could also skip over
NUL characters used as separators within the buffer that aren't at the
end of the buffer, as in: "host\0id\0channel\0lun".  If you want to
continue to allow that unlikely usage, then my patch comparing p to the
end pointer would work better.

From 61dc8daf4b6aa149882e425d58f68d50182222be Mon Sep 17 00:00:00 2001
From: Tony Battersby <[e-mail]>
Date: Mon, 24 Jul 2023 14:25:40 -0400
Subject: [PATCH] scsi: fix legacy /proc parsing buffer overflow

(lightly modified commit message mostly by Linus Torvalds)

The parsing code for /proc/scsi/scsi is disgusting and broken.  We
should have just used 'sscanf()' or something simple like that, but the
logic may actually predate our kernel sscanf library routine for all I
know.  It certainly predates both git and BK histories.

And we can't change it to be something sane like that now, because the
string matching at the start is done case-insensitively, and the
separator parsing between numbers isn't done at all, so *any* separator
will work, including a possible terminating NUL character.

This interface is root-only, and entirely for legacy use, so there is
absolutely no point in trying to tighten up the parsing.  Because any
separator has traditionally worked, it's entirely possible that people
have used random characters rather than the suggested space.

So don't bother to try to pretty it up, and let's just make a minimal
patch that can be back-ported and we can forget about this whole sorry
thing for another two decades.

Just make it at least not read past the end of the supplied data.

Link: https://lore.kernel.org/linux-scsi/[e-mail]/
Cc: Linus Torvalds <[e-mail]>
Cc: Martin K Petersen <[e-mail]>
Cc: James Bottomley <[e-mail]>
Cc: Willy Tarreau <[e-mail]>
Cc: [e-mail]
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Tony Battersby <[e-mail]>
---
 drivers/scsi/scsi_proc.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 4a6eb1741be0..41f23cd0bfb4 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -406,7 +406,7 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
 			       size_t length, loff_t *ppos)
 {
 	int host, channel, id, lun;
-	char *buffer, *p;
+	char *buffer, *end, *p;
 	int err;
 
 	if (!buf || length > PAGE_SIZE)
@@ -421,10 +421,14 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
 		goto out;
 
 	err = -EINVAL;
-	if (length < PAGE_SIZE)
-		buffer[length] = '\0';
-	else if (buffer[PAGE_SIZE-1])
-		goto out;
+	if (length < PAGE_SIZE) {
+		end = buffer + length;
+		*end = '\0';
+	} else {
+		end = buffer + PAGE_SIZE - 1;
+		if (*end)
+			goto out;
+	}
 
 	/*
 	 * Usage: echo "scsi add-single-device 0 1 2 3" >/proc/scsi/scsi
@@ -433,10 +437,10 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
 	if (!strncmp("scsi add-single-device", buffer, 22)) {
 		p = buffer + 23;
 
-		host = simple_strtoul(p, &p, 0);
-		channel = simple_strtoul(p + 1, &p, 0);
-		id = simple_strtoul(p + 1, &p, 0);
-		lun = simple_strtoul(p + 1, &p, 0);
+		host    = (p     < end) ? simple_strtoul(p, &p, 0) : 0;
+		channel = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
+		id      = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
+		lun     = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
 
 		err = scsi_add_single_device(host, channel, id, lun);
 
@@ -447,10 +451,10 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
 	} else if (!strncmp("scsi remove-single-device", buffer, 25)) {
 		p = buffer + 26;
 
-		host = simple_strtoul(p, &p, 0);
-		channel = simple_strtoul(p + 1, &p, 0);
-		id = simple_strtoul(p + 1, &p, 0);
-		lun = simple_strtoul(p + 1, &p, 0);
+		host    = (p     < end) ? simple_strtoul(p, &p, 0) : 0;
+		channel = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
+		id      = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
+		lun     = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
 
 		err = scsi_remove_single_device(host, channel, id, lun);
 	}

Linus Torvalds July 25, 2023, 6:41 p.m. UTC | #2

On Tue, 25 Jul 2023 at 11:27, Tony Battersby <[e-mail]> wrote:
>
> Something that I just thought of: the old parser could also skip over
> NUL characters used as separators within the buffer that aren't at the
> end of the buffer, as in: "host\0id\0channel\0lun".  If you want to
> continue to allow that unlikely usage, then my patch comparing p to the
> end pointer would work better.

Yeah, that would probably be better still. Ack on that.

That said, I just realized that *all* of this is completely
unnecessarily complicated. We allow up to a PAGE_SIZE, but you cannot
actually fill even *remotely* that much without using insane
zero-padding, and at that point you're not doing something useful,
you're trying to actively break something.

So the simple fix is to just limit the size of the buffer to slightly
less than PAGE_SIZE, and just pad more than one NUL character at the
end. Technically we're skipping four characters, and then we have the
last "real" NUL terminator, so 5 would be sufficient, but let's make
it easy for the compiler to just generate one single 64-bit store (or
two 32-bit ones) and clear 8 bytes.

IOW, we could do something *this* simple instead.

But I'm ok with your "track the end" version too.

             Linus

Patch

From 574fe269f5aaf62a3ec862bf430adf91a20823bd Mon Sep 17 00:00:00 2001
From: Linus Torvalds <[e-mail]>
Date: Tue, 25 Jul 2023 10:09:31 -0700
Subject: [PATCH] scsi: fix legacy /proc parsing buffer overflow

The parsing code for /proc/scsi/scsi is disgusting and broken.  We
should have just used 'sscanf()' or something simple like that, but the
logic may actually predate our kernel sscanf library routine for all I
know.  It certainly predates both git and BK histories.

And we can't change it to be something sane like that now, because the
string matching at the start is done case-insensitively, and the
separator parsing between numbers isn't done at all, so *any* separator
will work, including a possible terminating NUL character.

This interface is root-only, and entirely for legacy use, so there is
absolutely no point in trying to tighten up the parsing.  Because any
separator has traditionally worked, it's entirely possioble that people
have used random characters rather than the suggested space.

So don't bother to try to pretty it up, and let's just make a minimal
patch that can be back-ported and we can forget about this whole sorry
thing for another two decades.

Just make it at least not traverse the terminating NUL.

Reported-by: Tony Battersby <[e-mail]>
Link: https://lore.kernel.org/linux-scsi/[e-mail]/
Cc: Martin K Petersen <[e-mail]>
Cc: James Bottomley <[e-mail]>
Cc: Willy Tarreau <[e-mail]>
Cc: [e-mail]
Signed-off-by: Linus Torvalds <[e-mail]>
---
 drivers/scsi/scsi_proc.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 4a6eb1741be0..8aa8208ceb7f 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -383,6 +383,9 @@  static int scsi_remove_single_device(uint host, uint channel, uint id, uint lun)
 	return error;
 }
 
+/* increment 'p', but not past the end */
+static inline char *next_p(char *p) { return p + !!*p; }
+
 /**
  * proc_scsi_write - handle writes to /proc/scsi/scsi
  * @file: not used
@@ -431,12 +434,12 @@  static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
 	 * with  "0 1 2 3" replaced by your "Host Channel Id Lun".
 	 */
 	if (!strncmp("scsi add-single-device", buffer, 22)) {
-		p = buffer + 23;
+		p = buffer + 22;
 
-		host = simple_strtoul(p, &p, 0);
-		channel = simple_strtoul(p + 1, &p, 0);
-		id = simple_strtoul(p + 1, &p, 0);
-		lun = simple_strtoul(p + 1, &p, 0);
+		host = simple_strtoul(next_p(p), &p, 0);
+		channel = simple_strtoul(next_p(p), &p, 0);
+		id = simple_strtoul(next_p(p), &p, 0);
+		lun = simple_strtoul(next_p(p), &p, 0);
 
 		err = scsi_add_single_device(host, channel, id, lun);
 
@@ -445,12 +448,12 @@  static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
 	 * with  "0 1 2 3" replaced by your "Host Channel Id Lun".
 	 */
 	} else if (!strncmp("scsi remove-single-device", buffer, 25)) {
-		p = buffer + 26;
+		p = buffer + 25;
 
-		host = simple_strtoul(p, &p, 0);
-		channel = simple_strtoul(p + 1, &p, 0);
-		id = simple_strtoul(p + 1, &p, 0);
-		lun = simple_strtoul(p + 1, &p, 0);
+		host = simple_strtoul(next_p(p), &p, 0);
+		channel = simple_strtoul(next_p(p), &p, 0);
+		id = simple_strtoul(next_p(p), &p, 0);
+		lun = simple_strtoul(next_p(p), &p, 0);
 
 		err = scsi_remove_single_device(host, channel, id, lun);
 	}
-- 
2.41.0.327.gaa9166bcc0