| Message ID | 7733643d-e102-4581-8d29-769472011c97@moroto.mountain |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] scsi: scsi_debug: scsi: scsi_debug: fix some bugs in sdebug_error_write() | expand |
On 2023/11/6 22:04, Dan Carpenter wrote: > There are two bug in this code: > 1) If count is zero, then it will lead to a NULL dereference. The > kmalloc() will successfully allocate zero bytes and the test for > "if (buf[0] == '-')" will read beyond the end of the zero size buffer > and Oops. > 2) The code does not ensure that the user's string is properly NUL > terminated which could lead to a read overflow. > > Fixes: a9996d722b11 ("scsi: scsi_debug: Add interface to manage error injection for a single device") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > v2: At first I tried to use strndup_user() but that only accepts NUL > terminated strings and the user string is normally not terminated. > > drivers/scsi/scsi_debug.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c > index 67922e2c4c19..0dd21598f7b6 100644 > --- a/drivers/scsi/scsi_debug.c > +++ b/drivers/scsi/scsi_debug.c > @@ -1019,7 +1019,7 @@ static ssize_t sdebug_error_write(struct file *file, const char __user *ubuf, > struct sdebug_err_inject *inject; > struct scsi_device *sdev = (struct scsi_device *)file->f_inode->i_private; > > - buf = kmalloc(count, GFP_KERNEL); > + buf = kzalloc(count + 1, GFP_KERNEL); > if (!buf) > return -ENOMEM; > Looks good, Reviewed-by: Wenchao Hao <haowenchao2@huawei.com>
Dan, > There are two bug in this code: > 1) If count is zero, then it will lead to a NULL dereference. The > kmalloc() will successfully allocate zero bytes and the test for > "if (buf[0] == '-')" will read beyond the end of the zero size buffer > and Oops. > 2) The code does not ensure that the user's string is properly NUL > terminated which could lead to a read overflow. Applied 1+2 to 6.7/scsi-staging, thanks!
On Mon, 06 Nov 2023 17:04:33 +0300, Dan Carpenter wrote: > There are two bug in this code: > 1) If count is zero, then it will lead to a NULL dereference. The > kmalloc() will successfully allocate zero bytes and the test for > "if (buf[0] == '-')" will read beyond the end of the zero size buffer > and Oops. > 2) The code does not ensure that the user's string is properly NUL > terminated which could lead to a read overflow. > > [...] Applied to 6.7/scsi-fixes, thanks! [1/2] scsi: scsi_debug: scsi: scsi_debug: fix some bugs in sdebug_error_write() https://git.kernel.org/mkp/scsi/c/860c3d03bbc3 [2/2] scsi: scsi_debug: delete some bogus error checking https://git.kernel.org/mkp/scsi/c/037fbd3fcfbd
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index 67922e2c4c19..0dd21598f7b6 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -1019,7 +1019,7 @@ static ssize_t sdebug_error_write(struct file *file, const char __user *ubuf, struct sdebug_err_inject *inject; struct scsi_device *sdev = (struct scsi_device *)file->f_inode->i_private; - buf = kmalloc(count, GFP_KERNEL); + buf = kzalloc(count + 1, GFP_KERNEL); if (!buf) return -ENOMEM;
There are two bug in this code: 1) If count is zero, then it will lead to a NULL dereference. The kmalloc() will successfully allocate zero bytes and the test for "if (buf[0] == '-')" will read beyond the end of the zero size buffer and Oops. 2) The code does not ensure that the user's string is properly NUL terminated which could lead to a read overflow. Fixes: a9996d722b11 ("scsi: scsi_debug: Add interface to manage error injection for a single device") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- v2: At first I tried to use strndup_user() but that only accepts NUL terminated strings and the user string is normally not terminated. drivers/scsi/scsi_debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)