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

[Wenchao Hao <haowenchao22@gmail.com>]

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