[PATCH] scsi: isci: Fix a missing-check bug
In isci_request_oprom(), a for loop is used to find the OEM table by scanning the signature, which has four bytes. In each iteration, the signature is copied from the IO memory region 'oprom + i' to 'oem_sig' through memcpy_fromio(). Then 'oem_sig' is checked to see whether it is ISCI_OEM_SIG. If yes, the OEM table is found. Next, the header of the rom, including the signature, is then copied to 'oem_hdr' through memcpy_fromio(). It is obvious that the signature is copied twice here. Given that the device also has the permission to access the IO memory region, it is possible that a malicious device controlled by an attacker can modify the signature between these two copies. By doing so, the attacker can supply unexpected signatures, which can cause undefined behavior of the kernel and introduce potential security risk. This patch rewrites the signature after the second copy, using the value obtained in the first copy, and thus avoids the above issue. Signed-off-by: Wenwen Wang --- drivers/scsi/isci/probe_roms.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/isci/probe_roms.c b/drivers/scsi/isci/probe_roms.c index a2bbe46..bff54f2 100644 --- a/drivers/scsi/isci/probe_roms.c +++ b/drivers/scsi/isci/probe_roms.c @@ -68,6 +68,7 @@ struct isci_orom *isci_request_oprom(struct pci_dev *pdev) size_t copy_len; memcpy_fromio(&oem_hdr, oprom + i, sizeof(oem_hdr)); + memcpy(&oem_hdr.sig, oem_sig, ISCI_OEM_SIG_SIZE); copy_len = min(oem_hdr.len - sizeof(oem_hdr), sizeof(*rom)); -- 2.7.4
[PATCH] scsi: megaraid_sas: fix a missing-check bug
In megasas_mgmt_compat_ioctl_fw(), to handle the structure
compat_megasas_iocpacket 'cioc', a user-space structure megasas_iocpacket
'ioc' is allocated before megasas_mgmt_ioctl_fw() is invoked to handle the
packet. Since the two data structures have different fields, the data is
copied from 'cioc' to 'ioc' field by field. In the copy process,
'sense_ptr' is prepared if the field 'sense_len' is not null, because it
will be used in megasas_mgmt_ioctl_fw(). To prepare 'sense_ptr', the
user-space data 'ioc->sense_off' and 'cioc->sense_off' are copied and saved
to kernel-space variables 'local_sense_off' and 'user_sense_off'
respectively. Given that 'ioc->sense_off' is also copied from
'cioc->sense_off', 'local_sense_off' and 'user_sense_off' should have the
same value. However, 'cioc' is in the user space and a malicious user can
race to change the value of 'cioc->sense_off' after it is copied to
'ioc->sense_off' but before it is copied to 'user_sense_off'. By doing so,
the attacker can inject different values into 'local_sense_off' and
'user_sense_off'. This can cause undefined behavior in the following
execution, because the two variables are supposed to be same.
This patch enforces a check on the two kernel variables 'local_sense_off'
and 'user_sense_off' to make sure they are the same after the copy. In case
they are not, an error code EINVAL will be returned.
Signed-off-by: Wenwen Wang
---
drivers/scsi/megaraid/megaraid_sas_base.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
b/drivers/scsi/megaraid/megaraid_sas_base.c
index 9aa9590..f6de752 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -7523,6 +7523,9 @@ static int megasas_mgmt_compat_ioctl_fw(struct file
*file, unsigned long arg)
get_user(user_sense_off, &cioc->sense_off))
return -EFAULT;
+ if (local_sense_off != user_sense_off)
+ return -EINVAL;
+
if (local_sense_len) {
void __user **sense_ioc_ptr =
(void __user **)((u8 *)((unsigned long)&ioc->frame.raw)
+ local_sense_off);
--
2.7.4
Re: [PATCH] scsi: sg: fix a missing-check bug
On Mon, May 7, 2018 at 12:13 AM, Douglas Gilbert wrote: > On 2018-05-05 11:21 PM, Wenwen Wang wrote: >> >> In sg_write(), the opcode of the command is firstly copied from the >> userspace pointer 'buf' and saved to the kernel variable 'opcode', using >> the __get_user() function. The size of the command, i.e., 'cmd_size' is >> then calculated based on the 'opcode'. After that, the whole command, >> including the opcode, is copied again from 'buf' using the >> __copy_from_user() function and saved to 'cmnd'. Finally, the function >> sg_common_write() is invoked to process 'cmnd'. Given that the 'buf' >> pointer resides in userspace, a malicious userspace process can race to >> change the opcode of the command between the two copies. That means, the >> opcode indicated by the variable 'opcode' could be different from the >> opcode in 'cmnd'. This can cause inconsistent data in 'cmnd' and >> potential logical errors in the function sg_common_write(), as it needs to >> work on 'cmnd'. >> >> This patch reuses the opcode obtained in the first copy and only copies >> the >> remaining part of the command from userspace. >> >> Signed-off-by: Wenwen Wang >> --- >> drivers/scsi/sg.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >> index c198b963..0ad8106 100644 >> --- a/drivers/scsi/sg.c >> +++ b/drivers/scsi/sg.c >> @@ -657,7 +657,8 @@ sg_write(struct file *filp, const char __user *buf, >> size_t count, loff_t * ppos) >> hp->flags = input_size; /* structure abuse ... */ >> hp->pack_id = old_hdr.pack_id; >> hp->usr_ptr = NULL; >> - if (__copy_from_user(cmnd, buf, cmd_size)) >> + cmnd[0] = opcode; >> + if (__copy_from_user(cmnd + 1, buf + 1, cmd_size - 1)) >> return -EFAULT; >> /* >> * SG_DXFER_TO_FROM_DEV is functionally equivalent to >> SG_DXFER_FROM_DEV, >> > > That is in the deprecated "v2" part of the sg driver (for around 15 years). > There are lots more interesting races with that interface than that one > described above. My guess is that all system calls would be susceptible > to playing around with a buffer being passed to or from the OS by a thread > other than the one doing the system call, during that call. Surely no Unix > like OS gives any security guarantees to a thread being attacked by a > malevolent thread in the same process! > > My question is did this actually cause to program to fail; or is it > something > that a sanity checker flagged? This is detected by a static analysis tool. But, based on our manual investigation, it can cause program failure. So it is better to fix it. > > Also wouldn't it be better just to return an error such as EINVAL if > opcode != command[0] ? I can revise this patch to return EINVAL if the opcode is not as expected, and resubmit it. Thanks! Wenwen
Re: [PATCH] scsi: mpt3sas: fix a missing-check bug
Hello
Could you please review this patch? We need a confirmation because we
are working on an approaching deadline.
Thanks!
Wenwen
On Sat, May 5, 2018 at 1:31 AM, Wenwen Wang wrote:
> In _ctl_ioctl_main(), 'ioctl_header' is first copied from the userspace
> pointer 'arg'. 'ioctl_header.ioc_number' is then verified by
> _ctl_verify_adapter(). If the verification is failed, an error code -ENODEV
> is returned. Otherwise, the verification result, i.e., the MPT3SAS adapter
> that matches with the 'ioctl_header.ioc_number', is saved to 'ioc'. Later
> on, if the 'cmd' is MPT3COMMAND, the whole ioctl command struct is copied
> again from the userspace pointer 'arg' and saved to 'karg'. Then the
> function _ctl_do_mpt_command() is invoked to execute the command with the
> adapter 'ioc' and 'karg' as inputs.
>
> Given that the pointer 'arg' resides in userspace, a malicious userspace
> process can race to change the 'ioc_number' between the two copies, which
> will cause inconsistency issues, potentially security issues since an
> inconsistent adapter could be used with the command struct 'karg' as inputs
> of _ctl_do_mpt_command(). Moreover, the user can potentially provide a
> valid 'ioc_number' to pass the verification, and then modify it to an
> invalid 'ioc_number'. That means, an invalid 'ioc_number' can potentially
> bypass the verification check.
>
> To fix this issue, we need to recheck the 'ioc_number' copied after the
> second copy to make sure it is not changed since the first copy.
>
> Signed-off-by: Wenwen Wang
> ---
> drivers/scsi/mpt3sas/mpt3sas_ctl.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index d3cb387..0c140c7 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -2388,6 +2388,11 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd,
> void __user *arg,
> break;
> }
>
> + if (karg.hdr.ioc_number != ioctl_header.ioc_number) {
> + ret = -EINVAL;
> + break;
> + }
> +
> if (_IOC_SIZE(cmd) == sizeof(struct mpt3_ioctl_command)) {
> uarg = arg;
> ret = _ctl_do_mpt_command(ioc, karg, &uarg->mf);
> --
> 2.7.4
>
[PATCH v2] scsi: 3w-xxxx: fix a missing-check bug
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 for capable(CAP_SYS_ADMIN) in tw_chrdev_open() to avoid the above issues. Signed-off-by: Wenwen Wang --- drivers/scsi/3w-.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/scsi/3w-.c b/drivers/scsi/3w-.c index 33261b6..f6179e3 100644 --- a/drivers/scsi/3w-.c +++ b/drivers/scsi/3w-.c @@ -1033,6 +1033,9 @@ static int tw_chrdev_open(struct inode *inode, struct file *file) dprintk(KERN_WARNING "3w-: tw_ioctl_open()\n"); + if (!capable(CAP_SYS_ADMIN)) + return -EACCES; + minor_number = iminor(inode); if (minor_number >= tw_device_extension_count) return -ENODEV; -- 2.7.4
[PATCH v2] scsi: 3w-9xxx: fix a missing-check bug
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
---
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
[PATCH] scsi: 3ware: fix a missing-check bug
In twl_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 the buffer size 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
---
drivers/scsi/3w-sas.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c
index cf9f2a0..ea41969 100644
--- a/drivers/scsi/3w-sas.c
+++ b/drivers/scsi/3w-sas.c
@@ -757,6 +757,11 @@ static long twl_chrdev_ioctl(struct file *file, unsigned
int cmd, unsigned long
/* Now copy down the entire ioctl */
if (copy_from_user(tw_ioctl, argp, driver_command.buffer_length +
sizeof(TW_Ioctl_Buf_Apache) - 1))
goto out3;
+ if (tw_ioctl->driver_command.buffer_length !=
+ driver_command.buffer_length) {
+ retval = -EINVAL;
+ goto out3;
+ }
/* See which ioctl we are doing */
switch (cmd) {
--
2.7.4
[PATCH] scsi: 3w-xxxx: fix a missing-check bug
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
---
drivers/scsi/3w-.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/scsi/3w-.c b/drivers/scsi/3w-.c
index 33261b6..ef79194 100644
--- a/drivers/scsi/3w-.c
+++ b/drivers/scsi/3w-.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
[PATCH] scsi: 3w-9xxx: fix a missing-check bug
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 the buffer size 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
---
drivers/scsi/3w-9xxx.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index b42c9c4..8bc43db 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -684,6 +684,12 @@ static long twa_chrdev_ioctl(struct file *file, unsigned
int cmd, unsigned long
if (copy_from_user(tw_ioctl, argp, driver_command.buffer_length +
sizeof(TW_Ioctl_Buf_Apache) - 1))
goto out3;
+ if (tw_ioctl->driver_command.buffer_length
+!= driver_command.buffer_length) {
+ retval = TW_IOCTL_ERROR_OS_EINVAL;
+ goto out3;
+ }
+
/* See which ioctl we are doing */
switch (cmd) {
case TW_IOCTL_FIRMWARE_PASS_THROUGH:
--
2.7.4
[PATCH] scsi: sg: fix a missing-check bug
In sg_write(), the opcode of the command is firstly copied from the userspace pointer 'buf' and saved to the kernel variable 'opcode', using the __get_user() function. The size of the command, i.e., 'cmd_size' is then calculated based on the 'opcode'. After that, the whole command, including the opcode, is copied again from 'buf' using the __copy_from_user() function and saved to 'cmnd'. Finally, the function sg_common_write() is invoked to process 'cmnd'. Given that the 'buf' pointer resides in userspace, a malicious userspace process can race to change the opcode of the command between the two copies. That means, the opcode indicated by the variable 'opcode' could be different from the opcode in 'cmnd'. This can cause inconsistent data in 'cmnd' and potential logical errors in the function sg_common_write(), as it needs to work on 'cmnd'. This patch reuses the opcode obtained in the first copy and only copies the remaining part of the command from userspace. Signed-off-by: Wenwen Wang --- drivers/scsi/sg.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c index c198b963..0ad8106 100644 --- a/drivers/scsi/sg.c +++ b/drivers/scsi/sg.c @@ -657,7 +657,8 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos) hp->flags = input_size; /* structure abuse ... */ hp->pack_id = old_hdr.pack_id; hp->usr_ptr = NULL; - if (__copy_from_user(cmnd, buf, cmd_size)) + cmnd[0] = opcode; + if (__copy_from_user(cmnd + 1, buf + 1, cmd_size - 1)) return -EFAULT; /* * SG_DXFER_TO_FROM_DEV is functionally equivalent to SG_DXFER_FROM_DEV, -- 2.7.4
[PATCH] scsi: mpt3sas: fix a missing-check bug
In _ctl_ioctl_main(), 'ioctl_header' is first copied from the userspace
pointer 'arg'. 'ioctl_header.ioc_number' is then verified by
_ctl_verify_adapter(). If the verification is failed, an error code -ENODEV
is returned. Otherwise, the verification result, i.e., the MPT3SAS adapter
that matches with the 'ioctl_header.ioc_number', is saved to 'ioc'. Later
on, if the 'cmd' is MPT3COMMAND, the whole ioctl command struct is copied
again from the userspace pointer 'arg' and saved to 'karg'. Then the
function _ctl_do_mpt_command() is invoked to execute the command with the
adapter 'ioc' and 'karg' as inputs.
Given that the pointer 'arg' resides in userspace, a malicious userspace
process can race to change the 'ioc_number' between the two copies, which
will cause inconsistency issues, potentially security issues since an
inconsistent adapter could be used with the command struct 'karg' as inputs
of _ctl_do_mpt_command(). Moreover, the user can potentially provide a
valid 'ioc_number' to pass the verification, and then modify it to an
invalid 'ioc_number'. That means, an invalid 'ioc_number' can potentially
bypass the verification check.
To fix this issue, we need to recheck the 'ioc_number' copied after the
second copy to make sure it is not changed since the first copy.
Signed-off-by: Wenwen Wang
---
drivers/scsi/mpt3sas/mpt3sas_ctl.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index d3cb387..0c140c7 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -2388,6 +2388,11 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd,
void __user *arg,
break;
}
+ if (karg.hdr.ioc_number != ioctl_header.ioc_number) {
+ ret = -EINVAL;
+ break;
+ }
+
if (_IOC_SIZE(cmd) == sizeof(struct mpt3_ioctl_command)) {
uarg = arg;
ret = _ctl_do_mpt_command(ioc, karg, &uarg->mf);
--
2.7.4
