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

[PATCH 1/2] 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.

Fortunately, this is debugfs code and only root can write to it so
the security impact of these bugs is negligable.

Fixes: a9996d722b11 ("scsi: scsi_debug: Add interface to manage error injection for a single device")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/scsi/scsi_debug.c                      | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 67922e2c4c19..0a4e41d84df8 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1019,14 +1019,9 @@ 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);
-	if (!buf)
-		return -ENOMEM;
-
-	if (copy_from_user(buf, ubuf, count)) {
-		kfree(buf);
-		return -EFAULT;
-	}
+	buf = strndup_user(ubuf, count + 1);
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
 
 	if (buf[0] == '-')
 		return sdebug_err_remove(sdev, buf, count);
-- 
2.42.0

[PATCH 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 so
this checking can just be deleted.

Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
See my blog for more information on the history of debugfs error
checking:

https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
---
 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 0a4e41d84df8..c0be9a53ac79 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1127,7 +1127,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)
@@ -1135,15 +1134,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 2/2] scsi: scsi_debug: delete some bogus error checking

[Wenchao Hao]

On 2023/10/20 22:15, 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 so
> this checking can just be deleted.
> 
> Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> See my blog for more information on the history of debugfs error
> checking:
> 
> https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
> ---
>  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 0a4e41d84df8..c0be9a53ac79 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -1127,7 +1127,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)
> @@ -1135,15 +1134,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;
>  


Thank you for the fix, the check for debugfs_create_file() is added because 
scsi_debug driver is often used to test abnormal situations, here just check
and prompt a log, so maybe you should not remove it and fix the issue
following changes:

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 67922e2c4c19..d37437fa9eba 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1144,8 +1144,8 @@ static int sdebug_target_alloc(struct scsi_target *starget)
                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);
+       dentry = 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));

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

[Dan Carpenter]

On Sat, Oct 21, 2023 at 01:28:50AM +0800, Wenchao Hao wrote:
> On 2023/10/20 22:15, 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 so
> > this checking can just be deleted.
> > 
> > Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > See my blog for more information on the history of debugfs error
> > checking:
> > 
> > https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
> > ---
> >  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 0a4e41d84df8..c0be9a53ac79 100644
> > --- a/drivers/scsi/scsi_debug.c
> > +++ b/drivers/scsi/scsi_debug.c
> > @@ -1127,7 +1127,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)
> > @@ -1135,15 +1134,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;
> >  
> 
> 
> Thank you for the fix, the check for debugfs_create_file() is added because 
> scsi_debug driver is often used to test abnormal situations, here just check
> and prompt a log, so maybe you should not remove it and fix the issue
> following changes:
> 

No, the correct thing is to remove it.  This is explained in my blog
article linked to earlier.

https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/

commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date:   Wed Jan 23 11:28:14 2019 +0100

    debugfs: return error values, not NULL
    
    When an error happens, debugfs should return an error pointer value, not
    NULL.  This will prevent the totally theoretical error where a debugfs
    call fails due to lack of memory, returning NULL, and that dentry value
    is then passed to another debugfs call, which would end up succeeding,
    creating a file at the root of the debugfs tree, but would then be
    impossible to remove (because you can not remove the directory NULL).
    
    So, to make everyone happy, always return errors, this makes the users
    of debugfs much simpler (they do not have to ever check the return
    value), and everyone can rest easy.

In your code, if there is an error the debugfs code will print an error and
your code will print an info.  The info adds nothing.  Also if debugfs fails
to load you are already screwed so the info adds nothing.

In your code if the user disables CONFIG_DEBUGFS then printing "failed to create
fail_reset file for target" is wrong.  The user did that deliberately.  No need
to complain about the user's deliberate choices.  If it's really necessary to
have CONFIG_DEBUGFS then enforce that with Kconfig.

regards,
dan carpenter

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

[Wenchao Hao]

On 10/23/23 1:06 PM, Dan Carpenter wrote:
> On Sat, Oct 21, 2023 at 01:28:50AM +0800, Wenchao Hao wrote:
>> On 2023/10/20 22:15, 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 so
>>> this checking can just be deleted.
>>>
>>> Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> ---
>>> See my blog for more information on the history of debugfs error
>>> checking:
>>>
>>> https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
>>> ---
>>>  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 0a4e41d84df8..c0be9a53ac79 100644
>>> --- a/drivers/scsi/scsi_debug.c
>>> +++ b/drivers/scsi/scsi_debug.c
>>> @@ -1127,7 +1127,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)
>>> @@ -1135,15 +1134,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;
>>>  
>>
>>
>> Thank you for the fix, the check for debugfs_create_file() is added because 
>> scsi_debug driver is often used to test abnormal situations, here just check
>> and prompt a log, so maybe you should not remove it and fix the issue
>> following changes:
>>
> 
> No, the correct thing is to remove it.  This is explained in my blog
> article linked to earlier.
> 
> https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
> 

There are other places in scsi_debug which check return value
of debugfs functions added by my previous patches, would you
remove them?

Thanks


> commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
> Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date:   Wed Jan 23 11:28:14 2019 +0100
> 
>     debugfs: return error values, not NULL
>     
>     When an error happens, debugfs should return an error pointer value, not
>     NULL.  This will prevent the totally theoretical error where a debugfs
>     call fails due to lack of memory, returning NULL, and that dentry value
>     is then passed to another debugfs call, which would end up succeeding,
>     creating a file at the root of the debugfs tree, but would then be
>     impossible to remove (because you can not remove the directory NULL).
>     
>     So, to make everyone happy, always return errors, this makes the users
>     of debugfs much simpler (they do not have to ever check the return
>     value), and everyone can rest easy.
> 
> In your code, if there is an error the debugfs code will print an error and
> your code will print an info.  The info adds nothing.  Also if debugfs fails
> to load you are already screwed so the info adds nothing.
> 
> In your code if the user disables CONFIG_DEBUGFS then printing "failed to create
> fail_reset file for target" is wrong.  The user did that deliberately.  No need
> to complain about the user's deliberate choices.  If it's really necessary to
> have CONFIG_DEBUGFS then enforce that with Kconfig.
> 
> regards,
> dan carpenter

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

[Dan Carpenter]

On Wed, Oct 25, 2023 at 12:49:48AM +0800, Wenchao Hao wrote:
> On 10/23/23 1:06 PM, Dan Carpenter wrote:
> > On Sat, Oct 21, 2023 at 01:28:50AM +0800, Wenchao Hao wrote:
> >> On 2023/10/20 22:15, 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 so
> >>> this checking can just be deleted.
> >>>
> >>> Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
> >>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >>> ---
> >>> See my blog for more information on the history of debugfs error
> >>> checking:
> >>>
> >>> https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
> >>> ---
> >>>  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 0a4e41d84df8..c0be9a53ac79 100644
> >>> --- a/drivers/scsi/scsi_debug.c
> >>> +++ b/drivers/scsi/scsi_debug.c
> >>> @@ -1127,7 +1127,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)
> >>> @@ -1135,15 +1134,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;
> >>>  
> >>
> >>
> >> Thank you for the fix, the check for debugfs_create_file() is added because 
> >> scsi_debug driver is often used to test abnormal situations, here just check
> >> and prompt a log, so maybe you should not remove it and fix the issue
> >> following changes:
> >>
> > 
> > No, the correct thing is to remove it.  This is explained in my blog
> > article linked to earlier.
> > 
> > https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
> > 
> 
> There are other places in scsi_debug which check return value
> of debugfs functions added by my previous patches, would you
> remove them?

It's harmless to check most of the time.  Technically, they should be
removed but that's like a whitespace clean up so it's not worth spending
a lot of time on unless you care about a specific driver.  Here, though,
it was an uninitialized variable bug, and try to fix bugs.

Smatch is really about fixing bugs so I have special code to not warn
about if people check debugfs code for NULL.  Normally Smatch would
complain if people check an error pointer function for NULL but with
debugfs functions it doesn't matter.

regards,
dan carpenter

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

[Wenchao Hao]

[-- Attachment #1: Type: text/plain, Size: 2782 bytes --]

On Fri, Oct 20, 2023 at 10:15 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> There are two bug in this code:

Thanks for your fix, some different points of view as follows.

> 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.

This sysfs interface is usually used by cmdline, mostly, "echo" is used
to write it and "echo" always writes with '\n' terminated, which would
not cause a write with count=0.

While in terms of security, we should add a check for count==0
condition and return EINVAL.

> 2) The code does not ensure that the user's string is properly NUL
> terminated which could lead to a read overflow.
>

I don't think so, the copy_from_user() would limit the accessed length
to count, so no read overflow would happen.

Userspace's write would allocate a buffer larger than it actually
needed(usually 4K), but the buffer would not be cleared, so some
dirty data would be passed to the kernel space.

We might have following pairs of parameters for sdebug_error_write:

ubuf: "0 -10 0x12\n0 0 0x2 0x6 0x4 0x2"
count=11

the valid data in ubuf is "0 -10 -x12\n", others are dirty data.
strndup_user() would return EINVAL for this pair which caused
a correct write to fail.

You can recurrent the above error with my script attached.

Thanks.

> Fortunately, this is debugfs code and only root can write to it so
> the security impact of these bugs is negligable.
>
> Fixes: a9996d722b11 ("scsi: scsi_debug: Add interface to manage error injection for a single device")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/scsi/scsi_debug.c                      | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 67922e2c4c19..0a4e41d84df8 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -1019,14 +1019,9 @@ 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);
> -       if (!buf)
> -               return -ENOMEM;
> -
> -       if (copy_from_user(buf, ubuf, count)) {
> -               kfree(buf);
> -               return -EFAULT;
> -       }
> +       buf = strndup_user(ubuf, count + 1);
> +       if (IS_ERR(buf))
> +               return PTR_ERR(buf);
>
>         if (buf[0] == '-')
>                 return sdebug_err_remove(sdev, buf, count);
> --
> 2.42.0
>

[-- Attachment #2: test.sh --]
[-- Type: text/x-sh, Size: 6114 bytes --]

function clear_disk_error()
{
	local str=$(lsscsi | grep scsi_debug | grep $1 | awk '{print $1}')
	local scsi_id=${str#*\[}
	local scsi_id=${scsi_id%\]*}
	local error=/sys/kernel/debug/scsi_debug/$scsi_id/error 
	local target_id=${scsi_id%\:*}

	tmpfile=$$_clear
	cat $error
	cat $error | grep -v Type | awk '{print $1,$3}' > $tmpfile 
	while read -r line; do echo "- $line" > $error; done < $tmpfile
	rm -rf $tmpfile

	echo 0 > /sys/kernel/debug/scsi_debug/target$target_id/fail_reset
}

function disk_id()
{
	local str=$(lsscsi | grep scsi_debug | grep $1 | awk '{print $1}')
	local scsi_id=${str#*\[}
	local scsi_id=${scsi_id%\]*}
	echo $error
}

function disk_target_id()
{
	local str=$(lsscsi | grep scsi_debug | grep $1 | awk '{print $1}')
	local scsi_id=${str#*\[}
	local scsi_id=${scsi_id%\]*}
	local target_id=${scsi_id%\:*}

	echo $target_id
}

function disk_error()
{
	local str=$(lsscsi | grep scsi_debug | grep $1 | awk '{print $1}')
	local scsi_id=${str#*\[}
	local scsi_id=${scsi_id%\]*}
	local error=/sys/kernel/debug/scsi_debug/$scsi_id/error 

	echo $error
}

# time out and abort failed command
# would trigger error recovery from timeout
# $1: diskname
# $2: command name
# $3: rule time
function inject_TIMEOUT_ABORT()
{
	error=$(disk_error $1)
	echo "0 $3 $2" > ${error}
	echo "3 $3 $2" > ${error}
}

# finish command with DID_TIME_OUT
# would trigger error recovery when command finihed in scsi_decide_position
# $1: diskname
# $2: command name
# $3: rule time
function inject_DID_TIME_OUT()
{
	error=$(disk_error $1)
	echo "2 $3 $2 0x3 0 0 0 0 0" > ${error}
}

# finish command with LUN NOT READY, INITIALIZING COMMAND REQUIRED
# would trigger error recovery when command finihed in scsi_check_sense
# status: 0x2
# sense_key: 0x6
# asc: 0x4
# ascq: 0x2
# $1: diskname
# $2: command name
# $3: rule time
function inject_LUN_NOT_READY()
{
	error=$(disk_error $1)
	echo "2 $3 $2 0 0 0x2 0x6 0x4 0x2" > ${error}
}

# finish command with Medium Error
# would not trigger error recovery but EIO is triggered
# $1: diskname
# $2: command name
# $3: rule time
function inject_MEDIUM_ERROR()
{
	error=$(disk_error $1)
	echo "2 $3 $2 0 0 0x2 0x3 0x11 0x0" > ${error}
}

# device reset failed for 10 time
# $1: disk name to inject, for example sda
function recovery_inject1()
{
	error=$(disk_error $1)
	echo "4 -10 0xff" > ${error}
}

# target failed for 10 time
# $1: disk name to inject, for example sda
function recovery_inject2()
{
	target_id=$(disk_target_id $1)

	echo 1 > /sys/kernel/debug/scsi_debug/target$target_id/fail_reset
}

function recovery_inject3()
{
	inject_DID_TIME_OUT $1 0x12 -10
}

function recovery_inject4()
{
	inject_LUN_NOT_READY $1 0x12 -10
}

function recovery_inject5()
{
	inject_TIMEOUT_ABORT $1 0x12 -10
}

function recovery_inject6()
{
	inject_DID_TIME_OUT $1 0x1b -10
}

function recovery_inject7()
{
	inject_LUN_NOT_READY $1 0x1b -10
}

function recovery_inject8()
{
	inject_TIMEOUT_ABORT $1 0x1b -10
}

function error_inject1()
{
	inject_TIMEOUT_ABORT $1 0x28 -10
}

function error_inject2()
{
	inject_TIMEOUT_ABORT $1 0xa8 -10
}

function error_inject3()
{
	inject_TIMEOUT_ABORT $1 0x88 -10
}

function error_inject4()
{
	inject_TIMEOUT_ABORT $1 0x2a -10
}

function error_inject5()
{
	inject_TIMEOUT_ABORT $1 0xaa -10
}

function error_inject6()
{
	inject_TIMEOUT_ABORT $1 0x8a -10
}

function error_inject7()
{
	inject_TIMEOUT_ABORT $1 0x28 1
}

function error_inject8()
{
	inject_TIMEOUT_ABORT $1 0xa8 1
}

function error_inject9()
{
	inject_TIMEOUT_ABORT $1 0x88 1
}

function error_inject10()
{
	inject_TIMEOUT_ABORT $1 0x2a 1
}

function error_inject11()
{
	inject_TIMEOUT_ABORT $1 0xaa 1
}

function error_inject12()
{
	inject_TIMEOUT_ABORT $1 0x8a 1
}

function error_inject13()
{
	inject_DID_TIME_OUT $1 0x28 -10
}

function error_inject14()
{
	inject_DID_TIME_OUT $1 0xa8 -10
}

function error_inject15()
{
	inject_DID_TIME_OUT $1 0x88 -10
}

function error_inject16()
{
	inject_DID_TIME_OUT $1 0x2a -10
}

function error_inject17()
{
	inject_DID_TIME_OUT $1 0xaa -10
}

function error_inject18()
{
	inject_DID_TIME_OUT $1 0x8a -10
}

function error_inject19()
{
	inject_DID_TIME_OUT $1 0x28 1
}

function error_inject20()
{
	inject_DID_TIME_OUT $1 0xa8 1
}

function error_inject21()
{
	inject_DID_TIME_OUT $1 0x88 1
}

function error_inject22()
{
	inject_DID_TIME_OUT $1 0x2a 1
}

function error_inject23()
{
	inject_DID_TIME_OUT $1 0xaa 1
}

function error_inject24()
{
	inject_DID_TIME_OUT $1 0x8a 1
}

function error_inject25()
{
	inject_LUN_NOT_READY $1 0x28 -10
}

function error_inject26()
{
	inject_LUN_NOT_READY $1 0xa8 -10
}

function error_inject27()
{
	inject_LUN_NOT_READY $1 0x88 -10
}

function error_inject28()
{
	inject_LUN_NOT_READY $1 0x2a -10
}

function error_inject29()
{
	inject_LUN_NOT_READY $1 0xaa -10
}

function error_inject30()
{
	inject_LUN_NOT_READY $1 0x8a -10
}

function error_inject31()
{
	inject_LUN_NOT_READY $1 0x28 1
}

function error_inject32()
{
	inject_LUN_NOT_READY $1 0xa8 1
}

function error_inject33()
{
	inject_LUN_NOT_READY $1 0x88 1
}

function error_inject34()
{
	inject_LUN_NOT_READY $1 0x2a 1
}

function error_inject35()
{
	inject_LUN_NOT_READY $1 0xaa 1
}

function error_inject36()
{
	inject_LUN_NOT_READY $1 0x8a 1
}

function error_inject37()
{
	inject_MEDIUM_ERROR $1 0x28 -10
}

function error_inject38()
{
	inject_MEDIUM_ERROR $1 0xa8 -10
}

function error_inject39()
{
	inject_MEDIUM_ERROR $1 0x88 -10
}

function error_inject40()
{
	inject_MEDIUM_ERROR $1 0x2a -10
}

function error_inject41()
{
	inject_MEDIUM_ERROR $1 0xaa -10
}

function error_inject42()
{
	inject_MEDIUM_ERROR $1 0x8a -10
}

function error_inject43()
{
	inject_MEDIUM_ERROR $1 0x28 1
}

function error_inject44()
{
	inject_MEDIUM_ERROR $1 0xa8 1
}

function error_inject45()
{
	inject_MEDIUM_ERROR $1 0x88 1
}

function error_inject46()
{
	inject_MEDIUM_ERROR $1 0x2a 1
}

function error_inject47()
{
	inject_MEDIUM_ERROR $1 0xaa 1
}

function error_inject48()
{
	inject_MEDIUM_ERROR $1 0x8a 1
}

for i in $(seq 1 8);
do
	echo $i
	recovery_inject$i sda
done

for i in $(seq 1 48);
do
	echo $i
	error_inject$i sda
done

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

[Dan Carpenter]

On Sat, Oct 21, 2023 at 06:10:44PM +0800, Wenchao Hao wrote:
> On Fri, Oct 20, 2023 at 10:15 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > There are two bug in this code:
> 
> Thanks for your fix, some different points of view as follows.
> 
> > 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.
> 
> This sysfs interface is usually used by cmdline, mostly, "echo" is used
> to write it and "echo" always writes with '\n' terminated, which would
> not cause a write with count=0.
> 

You are saying "sysfs" but this is debugfs.  Sysfs is completely
different.  Also saying that 'and "echo" always writes with '\n'
terminated' is not true either even in sysfs...

> While in terms of security, we should add a check for count==0
> condition and return EINVAL.

Checking for zero is a valid approach.  I considered that but my way
was cleaner.

> 
> > 2) The code does not ensure that the user's string is properly NUL
> > terminated which could lead to a read overflow.
> >
> 
> I don't think so, the copy_from_user() would limit the accessed length
> to count, so no read overflow would happen.
> 
> Userspace's write would allocate a buffer larger than it actually
> needed(usually 4K), but the buffer would not be cleared, so some
> dirty data would be passed to the kernel space.
> 
> We might have following pairs of parameters for sdebug_error_write:
> 
> ubuf: "0 -10 0x12\n0 0 0x2 0x6 0x4 0x2"
> count=11
> 
> the valid data in ubuf is "0 -10 -x12\n", others are dirty data.
> strndup_user() would return EINVAL for this pair which caused
> a correct write to fail.
> 
> You can recurrent the above error with my script attached.

You're looking for the buffer overflow in the wrong place.

drivers/scsi/scsi_debug.c
  1026          if (copy_from_user(buf, ubuf, count)) {
                                   ^^^
We copy data from the user but it is not NUL terminated.

  1027                  kfree(buf);
  1028                  return -EFAULT;
  1029          }
  1030  
  1031          if (buf[0] == '-')
  1032                  return sdebug_err_remove(sdev, buf, count);
  1033  
  1034          if (sscanf(buf, "%d", &inject_type) != 1) {
                           ^^^
This will read beyond the end of the buffer.  sscanf() relies on a NUL
terminator to know when then end of the string is.

  1035                  kfree(buf);
  1036                  return -EINVAL;
  1037          }

Obviously the user in this situation is like a hacker who wants to do
something bad, not a normal users.  For a normal user this code is fine
as you say.

You will need to test this with .c code instead of shell if you want to
see the bug.

regards,
dan carpenter

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

[Wenchao Hao]

On 10/23/23 9:39 PM, Dan Carpenter wrote:
> On Sat, Oct 21, 2023 at 06:10:44PM +0800, Wenchao Hao wrote:
>> On Fri, Oct 20, 2023 at 10:15 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>>>
>>> There are two bug in this code:
>>
>> Thanks for your fix, some different points of view as follows.
>>
>>> 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.
>>
>> This sysfs interface is usually used by cmdline, mostly, "echo" is used
>> to write it and "echo" always writes with '\n' terminated, which would
>> not cause a write with count=0.
>>
> 
> You are saying "sysfs" but this is debugfs.  Sysfs is completely
> different.  Also saying that 'and "echo" always writes with '\n'
> terminated' is not true either even in sysfs...
> 
>> While in terms of security, we should add a check for count==0
>> condition and return EINVAL.
> 
> Checking for zero is a valid approach.  I considered that but my way
> was cleaner.
> 
>>
>>> 2) The code does not ensure that the user's string is properly NUL
>>> terminated which could lead to a read overflow.
>>>
>>
>> I don't think so, the copy_from_user() would limit the accessed length
>> to count, so no read overflow would happen.
>>
>> Userspace's write would allocate a buffer larger than it actually
>> needed(usually 4K), but the buffer would not be cleared, so some
>> dirty data would be passed to the kernel space.
>>
>> We might have following pairs of parameters for sdebug_error_write:
>>
>> ubuf: "0 -10 0x12\n0 0 0x2 0x6 0x4 0x2"
>> count=11
>>
>> the valid data in ubuf is "0 -10 -x12\n", others are dirty data.
>> strndup_user() would return EINVAL for this pair which caused
>> a correct write to fail.
>>
>> You can recurrent the above error with my script attached.
> 
> You're looking for the buffer overflow in the wrong place.
> 
> drivers/scsi/scsi_debug.c
>   1026          if (copy_from_user(buf, ubuf, count)) {
>                                    ^^^
> We copy data from the user but it is not NUL terminated.
> 
>   1027                  kfree(buf);
>   1028                  return -EFAULT;
>   1029          }
>   1030  
>   1031          if (buf[0] == '-')
>   1032                  return sdebug_err_remove(sdev, buf, count);
>   1033  
>   1034          if (sscanf(buf, "%d", &inject_type) != 1) {
>                            ^^^
> This will read beyond the end of the buffer.  sscanf() relies on a NUL
> terminator to know when then end of the string is.
> 
>   1035                  kfree(buf);
>   1036                  return -EINVAL;
>   1037          }
> 
> Obviously the user in this situation is like a hacker who wants to do
> something bad, not a normal users.  For a normal user this code is fine
> as you say.
> 
> You will need to test this with .c code instead of shell if you want to
> see the bug.
> 
> regards,
> dan carpenter
> 

Yes, there is bug here if write with .c code. Because your change to use
strndup_user() would make write with dirty data appended to "ubuf" failed,
can we fix it with following change:

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 67922e2c4c19..0e8ct724463f 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1019,7 +1019,7 @@ static seize_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;

Or is there other kernel lib function which can address this issue?

Thanks.

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

[Dan Carpenter]

On Wed, Oct 25, 2023 at 01:09:34AM +0800, Wenchao Hao wrote:
> Yes, there is bug here if write with .c code. Because your change to use
> strndup_user() would make write with dirty data appended to "ubuf" failed,

I don't understand this sentence.  What is "dirty" data in this context?

> can we fix it with following change:
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 67922e2c4c19..0e8ct724463f 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -1019,7 +1019,7 @@ static seize_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);

That would also fix the bug.

>         if (!buf)
>                 return -ENOMEM;
> 
> Or is there other kernel lib function which can address this issue?

I don't understand the issue.

regards,
dan carpenter

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

[Wenchao Hao]

On 2023/10/25 12:11, Dan Carpenter wrote:
> On Wed, Oct 25, 2023 at 01:09:34AM +0800, Wenchao Hao wrote:
>> Yes, there is bug here if write with .c code. Because your change to use
>> strndup_user() would make write with dirty data appended to "ubuf" failed,
> 
> I don't understand this sentence.  What is "dirty" data in this context?
> 

This is what I posted in previous reply:

We might have following pairs of parameters for sdebug_error_write:

ubuf: "0 -10 0x12\n0 0 0x2 0x6 0x4 0x2"
count=11

the valid data in ubuf is "0 -10 -x12\n", others are dirty data.
strndup_user() would return EINVAL for this pair which caused
a correct write to fail.

>> can we fix it with following change:
>>
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>> index 67922e2c4c19..0e8ct724463f 100644
>> --- a/drivers/scsi/scsi_debug.c
>> +++ b/drivers/scsi/scsi_debug.c
>> @@ -1019,7 +1019,7 @@ static seize_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);
> 
> That would also fix the bug.
> 
>>          if (!buf)
>>                  return -ENOMEM;
>>
>> Or is there other kernel lib function which can address this issue?
> 
> I don't understand the issue.
> 

I mean the bug you mentioned.

Thanks.

> regards,
> dan carpenter
> 
> 

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

[Dan Carpenter]

On Wed, Oct 25, 2023 at 02:10:41PM +0800, Wenchao Hao wrote:
> On 2023/10/25 12:11, Dan Carpenter wrote:
> > On Wed, Oct 25, 2023 at 01:09:34AM +0800, Wenchao Hao wrote:
> > > Yes, there is bug here if write with .c code. Because your change to use
> > > strndup_user() would make write with dirty data appended to "ubuf" failed,
> > 
> > I don't understand this sentence.  What is "dirty" data in this context?
> > 
> 
> This is what I posted in previous reply:
> 
> We might have following pairs of parameters for sdebug_error_write:
> 
> ubuf: "0 -10 0x12\n0 0 0x2 0x6 0x4 0x2"
> count=11
> 
> the valid data in ubuf is "0 -10 -x12\n", others are dirty data.
> strndup_user() would return EINVAL for this pair which caused
> a correct write to fail.

I mean, I could just tell you that your kzalloc(count + 1, GFP_KERNEL)
fix will work.  It does work.

But how is passing "dirty data" a "correct write"?  I feel like it
should be treated as incorrect and returning -EINVAL is the correct
behavior.  I'm so confused.  Why are users doing that?

I have looked at the code and it just doesn't seem that complicated.
They shouldn't be passing messed up strings and expect the kernel to
allow it.

regards,
dan carpenter

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

[Wenchao Hao]

On 10/25/23 3:07 PM, Dan Carpenter wrote:
> On Wed, Oct 25, 2023 at 02:10:41PM +0800, Wenchao Hao wrote:
>> On 2023/10/25 12:11, Dan Carpenter wrote:
>>> On Wed, Oct 25, 2023 at 01:09:34AM +0800, Wenchao Hao wrote:
>>>> Yes, there is bug here if write with .c code. Because your change to use
>>>> strndup_user() would make write with dirty data appended to "ubuf" failed,
>>>
>>> I don't understand this sentence.  What is "dirty" data in this context?
>>>
>>
>> This is what I posted in previous reply:
>>
>> We might have following pairs of parameters for sdebug_error_write:
>>
>> ubuf: "0 -10 0x12\n0 0 0x2 0x6 0x4 0x2"
>> count=11
>>
>> the valid data in ubuf is "0 -10 -x12\n", others are dirty data.
>> strndup_user() would return EINVAL for this pair which caused
>> a correct write to fail.
> 
> I mean, I could just tell you that your kzalloc(count + 1, GFP_KERNEL)
> fix will work.  It does work.
> 
> But how is passing "dirty data" a "correct write"?  I feel like it
> should be treated as incorrect and returning -EINVAL is the correct
> behavior.  I'm so confused.  Why are users doing that?
> 
> I have looked at the code and it just doesn't seem that complicated.
> They shouldn't be passing messed up strings and expect the kernel to
> allow it.
> 

First, echo command would call write() system call with string which is
terminated with '\n'. (I come to this conclusion with strace, but did not
check the source code of echo). So when we input echo "0 -10 0x12" > $error,
following pairs would be passed to sdebug_error_write:

ubuf: "0 -10 0x12\n"
count: 11

Second, it seems shell sh would not clean the dirty of previous execution.
For example, dirty data is passed to sdebug_error_write with following steps. 

echo "2 -10 0x1b 0 0 0x2 0x6 0x4 0x2" > /sys/kernel/debug/scsi_debug/3:0:0:0/error
echo "0 -10 0x1b" > /sys/kernel/debug/scsi_debug/3:0:0:0/error

I trace the parameters of sdebug_error_write with probe, following log is printed
when executing above two echo commands:

trace log:

# tracer: nop
#
# entries-in-buffer/entries-written: 2/2   #P:8
#
#                                _-----=> irqs-off/BH-disabled
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| / _-=> migrate-disable
#                              |||| /     delay
#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
              sh-13912   [007] ..... 482676.030303: sdebug_error_write: (sdebug_error_write+0x0/0x321 [scsi_debug]) comm="sh" count=31 ubuf="2 -10 0x1b 0 0 0x2 0x6 0x4 0x2
"
              sh-13912   [007] ..... 482676.030525: sdebug_error_write: (sdebug_error_write+0x0/0x321 [scsi_debug]) comm="sh" count=11 ubuf="0 -10 0x1b
0 0 0x2 0x6 0x4 0x2
"

Here is command to add kprobe trace:
echo 'p:sdebug_error_write sdebug_error_write comm=$comm count=$arg3:u64 ubuf=+0($arg2):ustring' >> /sys/kernel/debug/tracing/kprobe_events

It is proved that dirty data does exist, so I think we should now use strndup_user() here.

Thanks.

> regards,
> dan carpenter
> 

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

[Wenchao Hao]

On 11/4/23 2:00 AM, Wenchao Hao wrote:
> On 10/25/23 3:07 PM, Dan Carpenter wrote:
>> On Wed, Oct 25, 2023 at 02:10:41PM +0800, Wenchao Hao wrote:
>>> On 2023/10/25 12:11, Dan Carpenter wrote:
>>>> On Wed, Oct 25, 2023 at 01:09:34AM +0800, Wenchao Hao wrote:
>>>>> Yes, there is bug here if write with .c code. Because your change to use
>>>>> strndup_user() would make write with dirty data appended to "ubuf" failed,
>>>>
>>>> I don't understand this sentence.  What is "dirty" data in this context?
>>>>
>>>
>>> This is what I posted in previous reply:
>>>
>>> We might have following pairs of parameters for sdebug_error_write:
>>>
>>> ubuf: "0 -10 0x12\n0 0 0x2 0x6 0x4 0x2"
>>> count=11
>>>
>>> the valid data in ubuf is "0 -10 -x12\n", others are dirty data.
>>> strndup_user() would return EINVAL for this pair which caused
>>> a correct write to fail.
>>
>> I mean, I could just tell you that your kzalloc(count + 1, GFP_KERNEL)
>> fix will work.  It does work.
>>
>> But how is passing "dirty data" a "correct write"?  I feel like it
>> should be treated as incorrect and returning -EINVAL is the correct
>> behavior.  I'm so confused.  Why are users doing that?
>>
>> I have looked at the code and it just doesn't seem that complicated.
>> They shouldn't be passing messed up strings and expect the kernel to
>> allow it.
>>
> 
> First, echo command would call write() system call with string which is
> terminated with '\n'. (I come to this conclusion with strace, but did not
> check the source code of echo). So when we input echo "0 -10 0x12" > $error,
> following pairs would be passed to sdebug_error_write:
> 
> ubuf: "0 -10 0x12\n"
> count: 11
> 
> Second, it seems shell sh would not clean the dirty of previous execution.
> For example, dirty data is passed to sdebug_error_write with following steps. 
> 
> echo "2 -10 0x1b 0 0 0x2 0x6 0x4 0x2" > /sys/kernel/debug/scsi_debug/3:0:0:0/error
> echo "0 -10 0x1b" > /sys/kernel/debug/scsi_debug/3:0:0:0/error
> 
> I trace the parameters of sdebug_error_write with probe, following log is printed
> when executing above two echo commands:
> 
> trace log:
> 
> # tracer: nop
> #
> # entries-in-buffer/entries-written: 2/2   #P:8
> #
> #                                _-----=> irqs-off/BH-disabled
> #                               / _----=> need-resched
> #                              | / _---=> hardirq/softirq
> #                              || / _--=> preempt-depth
> #                              ||| / _-=> migrate-disable
> #                              |||| /     delay
> #           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
> #              | |         |   |||||     |         |
>               sh-13912   [007] ..... 482676.030303: sdebug_error_write: (sdebug_error_write+0x0/0x321 [scsi_debug]) comm="sh" count=31 ubuf="2 -10 0x1b 0 0 0x2 0x6 0x4 0x2
> "
>               sh-13912   [007] ..... 482676.030525: sdebug_error_write: (sdebug_error_write+0x0/0x321 [scsi_debug]) comm="sh" count=11 ubuf="0 -10 0x1b
> 0 0 0x2 0x6 0x4 0x2
> "
> 
> Here is command to add kprobe trace:
> echo 'p:sdebug_error_write sdebug_error_write comm=$comm count=$arg3:u64 ubuf=+0($arg2):ustring' >> /sys/kernel/debug/tracing/kprobe_events
> 
> It is proved that dirty data does exist, so I think we should now use strndup_user() here.

Sorry, its "should not use strndup_user()".

Thanks.

> 
> Thanks.
> 
>> regards,
>> dan carpenter
>>
> 

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

[Dan Carpenter]

Oh.  Duh.  The issue is that echo doesn't actually put a NUL terminator
on the end of the string...  Let's go with kzalloc(count + 1, as you
suggest.

regards,
dan carpenter