Skip to content

Commit 9426d3c

Browse files
abattersbymartinkpetersen
authored andcommitted
scsi: core: 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/b570f5fe-cb7c-863a-6ed9-f6774c219b88@cybernetics.com/ Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Martin K Petersen <martin.petersen@oracle.com> Cc: James Bottomley <jejb@linux.ibm.com> Cc: Willy Tarreau <w@1wt.eu> Cc: stable@kernel.org Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Tony Battersby <tonyb@cybernetics.com> Signed-off-by: Martin K Petersen <martin.petersen@oracle.com>
1 parent 8366d1f commit 9426d3c

File tree

1 file changed

+17
-13
lines changed

1 file changed

+17
-13
lines changed

drivers/scsi/scsi_proc.c

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,7 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
406406
size_t length, loff_t *ppos)
407407
{
408408
int host, channel, id, lun;
409-
char *buffer, *p;
409+
char *buffer, *end, *p;
410410
int err;
411411

412412
if (!buf || length > PAGE_SIZE)
@@ -421,10 +421,14 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
421421
goto out;
422422

423423
err = -EINVAL;
424-
if (length < PAGE_SIZE)
425-
buffer[length] = '\0';
426-
else if (buffer[PAGE_SIZE-1])
427-
goto out;
424+
if (length < PAGE_SIZE) {
425+
end = buffer + length;
426+
*end = '\0';
427+
} else {
428+
end = buffer + PAGE_SIZE - 1;
429+
if (*end)
430+
goto out;
431+
}
428432

429433
/*
430434
* 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,
433437
if (!strncmp("scsi add-single-device", buffer, 22)) {
434438
p = buffer + 23;
435439

436-
host = simple_strtoul(p, &p, 0);
437-
channel = simple_strtoul(p + 1, &p, 0);
438-
id = simple_strtoul(p + 1, &p, 0);
439-
lun = simple_strtoul(p + 1, &p, 0);
440+
host = (p < end) ? simple_strtoul(p, &p, 0) : 0;
441+
channel = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
442+
id = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
443+
lun = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
440444

441445
err = scsi_add_single_device(host, channel, id, lun);
442446

@@ -447,10 +451,10 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
447451
} else if (!strncmp("scsi remove-single-device", buffer, 25)) {
448452
p = buffer + 26;
449453

450-
host = simple_strtoul(p, &p, 0);
451-
channel = simple_strtoul(p + 1, &p, 0);
452-
id = simple_strtoul(p + 1, &p, 0);
453-
lun = simple_strtoul(p + 1, &p, 0);
454+
host = (p < end) ? simple_strtoul(p, &p, 0) : 0;
455+
channel = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
456+
id = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
457+
lun = (p + 1 < end) ? simple_strtoul(p + 1, &p, 0) : 0;
454458

455459
err = scsi_remove_single_device(host, channel, id, lun);
456460
}

0 commit comments

Comments
 (0)