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

[Dan Carpenter <dan.carpenter@linaro.org>]

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