[PATCH v2] scsi: 3w-9xxx: fix a missing-check bug

[PATCH v2] scsi: 3w-9xxx: fix a missing-check bug

[Wenwen Wang]

In twa_chrdev_ioctl(), the ioctl driver command is firstly copied from the
userspace pointer 'argp' and saved to the kernel object 'driver_command'.
Then a security check is performed on the data buffer size indicated by
'driver_command', which is 'driver_command.buffer_length'. If the security
check is passed, the entire ioctl command is copied again from the 'argp'
pointer and saved to the kernel object 'tw_ioctl'. Then, various operations
are performed on 'tw_ioctl' according to the 'cmd'. Given that the 'argp'
pointer resides in userspace, a malicious userspace process can race to
change the buffer size between the two copies. This way, the user can
bypass the security check and inject invalid data buffer size. This can
cause potential security issues in the following execution.

This patch checks for capable(CAP_SYS_ADMIN) in twa_chrdev_open()t o avoid
the above issues.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/scsi/3w-9xxx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index b42c9c4..99ba4a7 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -882,6 +882,11 @@ static int twa_chrdev_open(struct inode *inode, struct file *file)
 	unsigned int minor_number;
 	int retval = TW_IOCTL_ERROR_OS_ENODEV;
 
+	if (!capable(CAP_SYS_ADMIN)) {
+		retval = -EACCES;
+		goto out;
+	}
+
 	minor_number = iminor(inode);
 	if (minor_number >= twa_device_extension_count)
 		goto out;
-- 
2.7.4

Re: [PATCH v2] scsi: 3w-9xxx: fix a missing-check bug

[adam radford]

On Mon, May 7, 2018 at 5:46 PM, Wenwen Wang <wang6495@umn.edu> wrote:
> In twa_chrdev_ioctl(), the ioctl driver command is firstly copied from the
> userspace pointer 'argp' and saved to the kernel object 'driver_command'.
> Then a security check is performed on the data buffer size indicated by
> 'driver_command', which is 'driver_command.buffer_length'. If the security
> check is passed, the entire ioctl command is copied again from the 'argp'
> pointer and saved to the kernel object 'tw_ioctl'. Then, various operations
> are performed on 'tw_ioctl' according to the 'cmd'. Given that the 'argp'
> pointer resides in userspace, a malicious userspace process can race to
> change the buffer size between the two copies. This way, the user can
> bypass the security check and inject invalid data buffer size. This can
> cause potential security issues in the following execution.
>
> This patch checks for capable(CAP_SYS_ADMIN) in twa_chrdev_open()t o avoid
> the above issues.
>
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/scsi/3w-9xxx.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index b42c9c4..99ba4a7 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
> @@ -882,6 +882,11 @@ static int twa_chrdev_open(struct inode *inode, struct file *file)
>         unsigned int minor_number;
>         int retval = TW_IOCTL_ERROR_OS_ENODEV;
>
> +       if (!capable(CAP_SYS_ADMIN)) {
> +               retval = -EACCES;
> +               goto out;
> +       }
> +
>         minor_number = iminor(inode);
>         if (minor_number >= twa_device_extension_count)
>                 goto out;
> --
> 2.7.4
>

Acked-by: Adam Radford <aradford@gmail.com>

Re: [PATCH v2] scsi: 3w-9xxx: fix a missing-check bug

[Martin K. Petersen]

Wenwen,

> In twa_chrdev_ioctl(), the ioctl driver command is firstly copied from the
> userspace pointer 'argp' and saved to the kernel object 'driver_command'.
> Then a security check is performed on the data buffer size indicated by
> 'driver_command', which is 'driver_command.buffer_length'. If the security
> check is passed, the entire ioctl command is copied again from the 'argp'
> pointer and saved to the kernel object 'tw_ioctl'. Then, various operations
> are performed on 'tw_ioctl' according to the 'cmd'. Given that the 'argp'
> pointer resides in userspace, a malicious userspace process can race to
> change the buffer size between the two copies. This way, the user can
> bypass the security check and inject invalid data buffer size. This can
> cause potential security issues in the following execution.
>
> This patch checks for capable(CAP_SYS_ADMIN) in twa_chrdev_open()t o avoid
> the above issues.

Applied patch 1 + 2 to 4.18/scsi-queue. Thank you.

-- 
Martin K. Petersen	Oracle Linux Engineering