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
>
