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