diff mbox series

[v2,1/2] scsi: scsi_debug: scsi: scsi_debug: fix some bugs in sdebug_error_write()

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

Commit Message

Dan Carpenter Nov. 6, 2023, 2:04 p.m. UTC
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(-)

Comments

Wenchao Hao Nov. 7, 2023, 4:17 a.m. UTC | #1
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>
Martin K. Petersen Nov. 9, 2023, 2:42 a.m. UTC | #2
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!
Martin K. Petersen Nov. 15, 2023, 3:12 p.m. UTC | #3
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 mbox series

Patch

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;