LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] scsi: 3w-xxxx: fix a missing-check bug
@ 2018-05-06  5:48 Wenwen Wang
  2018-05-08  0:31 ` adam radford
  0 siblings, 1 reply; 2+ messages in thread
From: Wenwen Wang @ 2018-05-06  5:48 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, Adam Radford, James E.J. Bottomley,
	Martin K. Petersen, linux-scsi, open list

In tw_chrdev_ioctl(), the length of the data buffer is firstly copied from
the userspace pointer 'argp' and saved to the kernel object
'data_buffer_length'. Then a security check is performed on it to make sure
that the length is not more than 'TW_MAX_IOCTL_SECTORS * 512'. Otherwise,
an error code -EINVAL is returned. 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
length between the two copies. This way, the user can bypass the security
check and inject invalid data buffer length. This can cause potential
security issues in the following execution.

This patch checks the buffer length obtained in the second copy. An error
code -EINVAL will be returned if it is not same as the original one in the
first copy.

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

diff --git a/drivers/scsi/3w-xxxx.c b/drivers/scsi/3w-xxxx.c
index 33261b6..ef79194 100644
--- a/drivers/scsi/3w-xxxx.c
+++ b/drivers/scsi/3w-xxxx.c
@@ -919,6 +919,10 @@ static long tw_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long a
 	/* Now copy down the entire ioctl */
 	if (copy_from_user(tw_ioctl, argp, data_buffer_length + sizeof(TW_New_Ioctl) - 1))
 		goto out2;
+	if (tw_ioctl->data_buffer_length != data_buffer_length) {
+		retval = -EINVAL;
+		goto out2;
+	}
 
 	passthru = (TW_Passthru *)&tw_ioctl->firmware_command;
 
-- 
2.7.4

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] scsi: 3w-xxxx: fix a missing-check bug
  2018-05-06  5:48 [PATCH] scsi: 3w-xxxx: fix a missing-check bug Wenwen Wang
@ 2018-05-08  0:31 ` adam radford
  0 siblings, 0 replies; 2+ messages in thread
From: adam radford @ 2018-05-08  0:31 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
	open list

On Sat, May 5, 2018 at 10:48 PM, Wenwen Wang <wang6495@umn.edu> wrote:
> In tw_chrdev_ioctl(), the length of the data buffer is firstly copied from
> the userspace pointer 'argp' and saved to the kernel object
> 'data_buffer_length'. Then a security check is performed on it to make sure
> that the length is not more than 'TW_MAX_IOCTL_SECTORS * 512'. Otherwise,
> an error code -EINVAL is returned. 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
> length between the two copies. This way, the user can bypass the security
> check and inject invalid data buffer length. This can cause potential
> security issues in the following execution.
>
> This patch checks the buffer length obtained in the second copy. An error
> code -EINVAL will be returned if it is not same as the original one in the
> first copy.
>
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/scsi/3w-xxxx.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/scsi/3w-xxxx.c b/drivers/scsi/3w-xxxx.c
> index 33261b6..ef79194 100644
> --- a/drivers/scsi/3w-xxxx.c
> +++ b/drivers/scsi/3w-xxxx.c
> @@ -919,6 +919,10 @@ static long tw_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long a
>         /* Now copy down the entire ioctl */
>         if (copy_from_user(tw_ioctl, argp, data_buffer_length + sizeof(TW_New_Ioctl) - 1))
>                 goto out2;
> +       if (tw_ioctl->data_buffer_length != data_buffer_length) {
> +               retval = -EINVAL;
> +               goto out2;
> +       }
>
>         passthru = (TW_Passthru *)&tw_ioctl->firmware_command;
>
> --
> 2.7.4
>

I would drop this patch and check for !capable(CAP_SYS_ADMIN) in
tw_chrdev_open() instead.

-Adam

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-05-08  0:31 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-06  5:48 [PATCH] scsi: 3w-xxxx: fix a missing-check bug Wenwen Wang
2018-05-08  0:31 ` adam radford

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).