| Message ID | 96d50cf7-afec-46af-9d98-08099f8dc76e@moroto.mountain (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/2] scsi: scsi_debug: fix some bugs in sdebug_error_write() | expand |
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. While in terms of security, we should add a check for count==0 condition and return EINVAL. > 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. Thanks. > Fortunately, this is debugfs code and only root can write to it so > the security impact of these bugs is negligable. > > Fixes: a9996d722b11 ("scsi: scsi_debug: Add interface to manage error injection for a single device") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/scsi/scsi_debug.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c > index 67922e2c4c19..0a4e41d84df8 100644 > --- a/drivers/scsi/scsi_debug.c > +++ b/drivers/scsi/scsi_debug.c > @@ -1019,14 +1019,9 @@ static ssize_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); > - if (!buf) > - return -ENOMEM; > - > - if (copy_from_user(buf, ubuf, count)) { > - kfree(buf); > - return -EFAULT; > - } > + buf = strndup_user(ubuf, count + 1); > + if (IS_ERR(buf)) > + return PTR_ERR(buf); > > if (buf[0] == '-') > return sdebug_err_remove(sdev, buf, count); > -- > 2.42.0 >
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
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.
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? > 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); That would also fix the bug. > if (!buf) > return -ENOMEM; > > Or is there other kernel lib function which can address this issue? I don't understand the issue. regards, dan carpenter
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. >> 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); > > That would also fix the bug. > >> if (!buf) >> return -ENOMEM; >> >> Or is there other kernel lib function which can address this issue? > > I don't understand the issue. > I mean the bug you mentioned. Thanks. > regards, > dan carpenter > >
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. regards, dan carpenter
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 >
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 >> >
Oh. Duh. The issue is that echo doesn't actually put a NUL terminator on the end of the string... Let's go with kzalloc(count + 1, as you suggest. regards, dan carpenter
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index 67922e2c4c19..0a4e41d84df8 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -1019,14 +1019,9 @@ static ssize_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); - if (!buf) - return -ENOMEM; - - if (copy_from_user(buf, ubuf, count)) { - kfree(buf); - return -EFAULT; - } + buf = strndup_user(ubuf, count + 1); + if (IS_ERR(buf)) + return PTR_ERR(buf); if (buf[0] == '-') return sdebug_err_remove(sdev, buf, count);
There are two bug in this code: 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. 2) The code does not ensure that the user's string is properly NUL terminated which could lead to a read overflow. Fortunately, this is debugfs code and only root can write to it so the security impact of these bugs is negligable. Fixes: a9996d722b11 ("scsi: scsi_debug: Add interface to manage error injection for a single device") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/scsi/scsi_debug.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)