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

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

[Dan Carpenter]

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;
 
-- 
2.42.0

[PATCH v2 2/2] scsi: scsi_debug: delete some bogus error checking

[Dan Carpenter]

Smatch complains that "dentry" is never initialized.  These days everyone
initializes all their stack variables to zero so this means that it will
trigger a warning every time this function is run.

Really, debugfs functions are not supposed to be checked for errors in
normal code.  For example, if we updated this code to check the correct
variable then it would print a warning if CONFIG_DEBUGFS was disabled.
We don't want that.  Just delete the check.

Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
v2: Add some more text to the commit message about CONFIG_DEBUGFS

 drivers/scsi/scsi_debug.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 0dd21598f7b6..6d8218a44122 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1132,7 +1132,6 @@ static const struct file_operations sdebug_target_reset_fail_fops = {
 static int sdebug_target_alloc(struct scsi_target *starget)
 {
 	struct sdebug_target_info *targetip;
-	struct dentry *dentry;
 
 	targetip = kzalloc(sizeof(struct sdebug_target_info), GFP_KERNEL);
 	if (!targetip)
@@ -1140,15 +1139,9 @@ static int sdebug_target_alloc(struct scsi_target *starget)
 
 	targetip->debugfs_entry = debugfs_create_dir(dev_name(&starget->dev),
 				sdebug_debugfs_root);
-	if (IS_ERR_OR_NULL(targetip->debugfs_entry))
-		pr_info("%s: failed to create debugfs directory for target %s\n",
-			__func__, dev_name(&starget->dev));
 
 	debugfs_create_file("fail_reset", 0600, targetip->debugfs_entry, starget,
 				&sdebug_target_reset_fail_fops);
-	if (IS_ERR_OR_NULL(dentry))
-		pr_info("%s: failed to create fail_reset file for target %s\n",
-			__func__, dev_name(&starget->dev));
 
 	starget->hostdata = targetip;
 
-- 
2.42.0

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

[Wenchao Hao]

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>

Re: [PATCH v2 2/2] scsi: scsi_debug: delete some bogus error checking

[Wenchao Hao]

On 2023/11/6 22:05, Dan Carpenter wrote:
> Smatch complains that "dentry" is never initialized.  These days everyone
> initializes all their stack variables to zero so this means that it will
> trigger a warning every time this function is run.
> 
> Really, debugfs functions are not supposed to be checked for errors in
> normal code.  For example, if we updated this code to check the correct
> variable then it would print a warning if CONFIG_DEBUGFS was disabled.
> We don't want that.  Just delete the check.
> 
> Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> v2: Add some more text to the commit message about CONFIG_DEBUGFS
> 
>   drivers/scsi/scsi_debug.c | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 0dd21598f7b6..6d8218a44122 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -1132,7 +1132,6 @@ static const struct file_operations sdebug_target_reset_fail_fops = {
>   static int sdebug_target_alloc(struct scsi_target *starget)
>   {
>   	struct sdebug_target_info *targetip;
> -	struct dentry *dentry;
>   
>   	targetip = kzalloc(sizeof(struct sdebug_target_info), GFP_KERNEL);
>   	if (!targetip)
> @@ -1140,15 +1139,9 @@ static int sdebug_target_alloc(struct scsi_target *starget)
>   
>   	targetip->debugfs_entry = debugfs_create_dir(dev_name(&starget->dev),
>   				sdebug_debugfs_root);
> -	if (IS_ERR_OR_NULL(targetip->debugfs_entry))
> -		pr_info("%s: failed to create debugfs directory for target %s\n",
> -			__func__, dev_name(&starget->dev));
>   
>   	debugfs_create_file("fail_reset", 0600, targetip->debugfs_entry, starget,
>   				&sdebug_target_reset_fail_fops);
> -	if (IS_ERR_OR_NULL(dentry))
> -		pr_info("%s: failed to create fail_reset file for target %s\n",
> -			__func__, dev_name(&starget->dev));
>   
>   	starget->hostdata = targetip;
>   
Looks good, thanks for the fix.

Reviewed-by: Wenchao Hao <haowenchao2@huawei.com>

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

[Martin K. Petersen]

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	Oracle Linux Engineering

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

[Martin K. Petersen]

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

-- 
Martin K. Petersen	Oracle Linux Engineering