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