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

[Wenchao Hao <haowenchao22@gmail.com>]

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.