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

Wenchao Hao
2023-11-07T04:17:25Z

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>