On Fri, Oct 20, 2023 at 10:15 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
>
function clear_disk_error()
{
local str=$(lsscsi | grep scsi_debug | grep $1 | awk '{print $1}')
local scsi_id=${str#*\[}
local scsi_id=${scsi_id%\]*}
local error=/sys/kernel/debug/scsi_debug/$scsi_id/error
local target_id=${scsi_id%\:*}
tmpfile=$$_clear
cat $error
cat $error | grep -v Type | awk '{print $1,$3}' > $tmpfile
while read -r line; do echo "- $line" > $error; done < $tmpfile
rm -rf $tmpfile
echo 0 > /sys/kernel/debug/scsi_debug/target$target_id/fail_reset
}
function disk_id()
{
local str=$(lsscsi | grep scsi_debug | grep $1 | awk '{print $1}')
local scsi_id=${str#*\[}
local scsi_id=${scsi_id%\]*}
echo $error
}
function disk_target_id()
{
local str=$(lsscsi | grep scsi_debug | grep $1 | awk '{print $1}')
local scsi_id=${str#*\[}
local scsi_id=${scsi_id%\]*}
local target_id=${scsi_id%\:*}
echo $target_id
}
function disk_error()
{
local str=$(lsscsi | grep scsi_debug | grep $1 | awk '{print $1}')
local scsi_id=${str#*\[}
local scsi_id=${scsi_id%\]*}
local error=/sys/kernel/debug/scsi_debug/$scsi_id/error
echo $error
}
# time out and abort failed command
# would trigger error recovery from timeout
# $1: diskname
# $2: command name
# $3: rule time
function inject_TIMEOUT_ABORT()
{
error=$(disk_error $1)
echo "0 $3 $2" > ${error}
echo "3 $3 $2" > ${error}
}
# finish command with DID_TIME_OUT
# would trigger error recovery when command finihed in scsi_decide_position
# $1: diskname
# $2: command name
# $3: rule time
function inject_DID_TIME_OUT()
{
error=$(disk_error $1)
echo "2 $3 $2 0x3 0 0 0 0 0" > ${error}
}
# finish command with LUN NOT READY, INITIALIZING COMMAND REQUIRED
# would trigger error recovery when command finihed in scsi_check_sense
# status: 0x2
# sense_key: 0x6
# asc: 0x4
# ascq: 0x2
# $1: diskname
# $2: command name
# $3: rule time
function inject_LUN_NOT_READY()
{
error=$(disk_error $1)
echo "2 $3 $2 0 0 0x2 0x6 0x4 0x2" > ${error}
}
# finish command with Medium Error
# would not trigger error recovery but EIO is triggered
# $1: diskname
# $2: command name
# $3: rule time
function inject_MEDIUM_ERROR()
{
error=$(disk_error $1)
echo "2 $3 $2 0 0 0x2 0x3 0x11 0x0" > ${error}
}
# device reset failed for 10 time
# $1: disk name to inject, for example sda
function recovery_inject1()
{
error=$(disk_error $1)
echo "4 -10 0xff" > ${error}
}
# target failed for 10 time
# $1: disk name to inject, for example sda
function recovery_inject2()
{
target_id=$(disk_target_id $1)
echo 1 > /sys/kernel/debug/scsi_debug/target$target_id/fail_reset
}
function recovery_inject3()
{
inject_DID_TIME_OUT $1 0x12 -10
}
function recovery_inject4()
{
inject_LUN_NOT_READY $1 0x12 -10
}
function recovery_inject5()
{
inject_TIMEOUT_ABORT $1 0x12 -10
}
function recovery_inject6()
{
inject_DID_TIME_OUT $1 0x1b -10
}
function recovery_inject7()
{
inject_LUN_NOT_READY $1 0x1b -10
}
function recovery_inject8()
{
inject_TIMEOUT_ABORT $1 0x1b -10
}
function error_inject1()
{
inject_TIMEOUT_ABORT $1 0x28 -10
}
function error_inject2()
{
inject_TIMEOUT_ABORT $1 0xa8 -10
}
function error_inject3()
{
inject_TIMEOUT_ABORT $1 0x88 -10
}
function error_inject4()
{
inject_TIMEOUT_ABORT $1 0x2a -10
}
function error_inject5()
{
inject_TIMEOUT_ABORT $1 0xaa -10
}
function error_inject6()
{
inject_TIMEOUT_ABORT $1 0x8a -10
}
function error_inject7()
{
inject_TIMEOUT_ABORT $1 0x28 1
}
function error_inject8()
{
inject_TIMEOUT_ABORT $1 0xa8 1
}
function error_inject9()
{
inject_TIMEOUT_ABORT $1 0x88 1
}
function error_inject10()
{
inject_TIMEOUT_ABORT $1 0x2a 1
}
function error_inject11()
{
inject_TIMEOUT_ABORT $1 0xaa 1
}
function error_inject12()
{
inject_TIMEOUT_ABORT $1 0x8a 1
}
function error_inject13()
{
inject_DID_TIME_OUT $1 0x28 -10
}
function error_inject14()
{
inject_DID_TIME_OUT $1 0xa8 -10
}
function error_inject15()
{
inject_DID_TIME_OUT $1 0x88 -10
}
function error_inject16()
{
inject_DID_TIME_OUT $1 0x2a -10
}
function error_inject17()
{
inject_DID_TIME_OUT $1 0xaa -10
}
function error_inject18()
{
inject_DID_TIME_OUT $1 0x8a -10
}
function error_inject19()
{
inject_DID_TIME_OUT $1 0x28 1
}
function error_inject20()
{
inject_DID_TIME_OUT $1 0xa8 1
}
function error_inject21()
{
inject_DID_TIME_OUT $1 0x88 1
}
function error_inject22()
{
inject_DID_TIME_OUT $1 0x2a 1
}
function error_inject23()
{
inject_DID_TIME_OUT $1 0xaa 1
}
function error_inject24()
{
inject_DID_TIME_OUT $1 0x8a 1
}
function error_inject25()
{
inject_LUN_NOT_READY $1 0x28 -10
}
function error_inject26()
{
inject_LUN_NOT_READY $1 0xa8 -10
}
function error_inject27()
{
inject_LUN_NOT_READY $1 0x88 -10
}
function error_inject28()
{
inject_LUN_NOT_READY $1 0x2a -10
}
function error_inject29()
{
inject_LUN_NOT_READY $1 0xaa -10
}
function error_inject30()
{
inject_LUN_NOT_READY $1 0x8a -10
}
function error_inject31()
{
inject_LUN_NOT_READY $1 0x28 1
}
function error_inject32()
{
inject_LUN_NOT_READY $1 0xa8 1
}
function error_inject33()
{
inject_LUN_NOT_READY $1 0x88 1
}
function error_inject34()
{
inject_LUN_NOT_READY $1 0x2a 1
}
function error_inject35()
{
inject_LUN_NOT_READY $1 0xaa 1
}
function error_inject36()
{
inject_LUN_NOT_READY $1 0x8a 1
}
function error_inject37()
{
inject_MEDIUM_ERROR $1 0x28 -10
}
function error_inject38()
{
inject_MEDIUM_ERROR $1 0xa8 -10
}
function error_inject39()
{
inject_MEDIUM_ERROR $1 0x88 -10
}
function error_inject40()
{
inject_MEDIUM_ERROR $1 0x2a -10
}
function error_inject41()
{
inject_MEDIUM_ERROR $1 0xaa -10
}
function error_inject42()
{
inject_MEDIUM_ERROR $1 0x8a -10
}
function error_inject43()
{
inject_MEDIUM_ERROR $1 0x28 1
}
function error_inject44()
{
inject_MEDIUM_ERROR $1 0xa8 1
}
function error_inject45()
{
inject_MEDIUM_ERROR $1 0x88 1
}
function error_inject46()
{
inject_MEDIUM_ERROR $1 0x2a 1
}
function error_inject47()
{
inject_MEDIUM_ERROR $1 0xaa 1
}
function error_inject48()
{
inject_MEDIUM_ERROR $1 0x8a 1
}
for i in $(seq 1 8);
do
echo $i
recovery_inject$i sda
done
for i in $(seq 1 48);
do
echo $i
error_inject$i sda
done
