I am trying to fix a bug in the parser for these commands:
echo "scsi add-single-device host channel id lun" > /proc/scsi/scsi
echo "scsi remove-single-device host channel id lun" > /proc/scsi/scsi
With the current parser, if you omit some of the fields (for example the
lun), then the kernel will usually fill in the missing parameters with a
0, but on rare occasion it might supply something else. So my question
for linux-scsi is: does anyone rely on omitting some of the parameters
in the cmds above and expect the kernel to supply 0 for the missing
parameters (for example lun is often 0)? If so, then I can make the
parser always supply a 0 for the missing parameters. If not, then I can
make the parser return an error if there are paramters missing, on the
theory that guessing which device to add or remove is a bad idea.
Below is the patch to return an error for a missing parameter. The
patch to use 0 instead of returning an error is similar but intead of
goto uses:
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;
---
From 4f8f3291b18fddb2cf75581a2a2cf847fd2896a7 Mon Sep 17 00:00:00 2001
From: Tony Battersby <[email protected]>
Date: Fri, 21 Jul 2023 11:12:27 -0400
Subject: [PATCH] scsi: core: fix parsing of scsi {add,remove}-single-device
When parsing the "scsi add-single-device" and
"scsi remove-single-device" commands written to /proc/scsi/scsi, make
sure the parser doesn't skip over the NUL string terminator and read
past the end of the user-supplied string.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: [email protected]
Signed-off-by: Tony Battersby <[email protected]>
---
drivers/scsi/scsi_proc.c | 48 +++++++++++++++++++++++++++++++---------
1 file changed, 37 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 4a6eb1741be0..b27c8da83e62 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
@@ -432,11 +436,22 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
*/
if (!strncmp("scsi add-single-device", buffer, 22)) {
p = buffer + 23;
+ if (p >= end)
+ goto out;
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);
+ if (++p >= end)
+ goto out;
+
+ channel = simple_strtoul(p, &p, 0);
+ if (++p >= end)
+ goto out;
+
+ id = simple_strtoul(p, &p, 0);
+ if (++p >= end)
+ goto out;
+
+ lun = simple_strtoul(p, &p, 0);
err = scsi_add_single_device(host, channel, id, lun);
@@ -446,11 +461,22 @@ 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;
+ if (p >= end)
+ goto out;
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);
+ if (++p >= end)
+ goto out;
+
+ channel = simple_strtoul(p, &p, 0);
+ if (++p >= end)
+ goto out;
+
+ id = simple_strtoul(p, &p, 0);
+ if (++p >= end)
+ goto out;
+
+ lun = simple_strtoul(p, &p, 0);
err = scsi_remove_single_device(host, channel, id, lun);
}
--
2.25.1