Bug 1717182 (CVE-2019-12456) - CVE-2019-12456 kernel: double fetch in the MPT3COMMAND case in _ctl_ioctl_main in drivers/scsi/mpt3sas/mpt3sas_ctl.c
Summary: CVE-2019-12456 kernel: double fetch in the MPT3COMMAND case in _ctl_ioctl_mai...
Keywords:
Status: NEW
Alias: CVE-2019-12456
Product: Security Response
Classification: Other
Component: vulnerability
Version: unspecified
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Red Hat Product Security
QA Contact:
URL:
Whiteboard: impact=moderate,public=20190529,repor...
Depends On: 1717183
Blocks: 1717184
TreeView+ depends on / blocked
 
Reported: 2019-06-04 20:35 UTC by Laura Pardo
Modified: 2019-06-20 06:12 UTC (History)
44 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:


Attachments (Terms of Use)

Description Laura Pardo 2019-06-04 20:35:56 UTC
An issue was discovered in the MPT3COMMAND case in _ctl_ioctl_main in drivers/scsi/mpt3sas/mpt3sas_ctl.c in the Linux kernel through 5.1.5. It allows local users to cause a denial of service by changing the value of ioc_number between two kernel reads of that value, aka a "double fetch" vulnerability.


References:
https://lkml.org/lkml/2019/5/29/1164

Upstream Patch:
https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=5.3/scsi-queue&id=86e5aca7fa2927060839f3e3b40c8bd65a7e8d1e

Comment 1 Laura Pardo 2019-06-04 20:37:17 UTC
Created kernel tracking bugs for this issue:

Affects: fedora-all [bug 1717183]

Comment 2 Wade Mealing 2019-06-17 06:21:20 UTC
Initial investigation of this flaw does not conclude that this flaw is exploitable.  Upstream discussion on this flaw will continue.

Comment 3 Wade Mealing 2019-06-20 03:07:33 UTC
My investigation of this flaw, while no flaw is found it might be useful
if anyone is following along.

The reporter classifies this flaw as a 'double fetch'.  The typical
Double Fetch vulnerability is usually when a user-mode thread prepares
data and enters the kernel through a system call. The data is fetched
twice in the kernel (copy_from_user), and the kernel first fetches data
for security check ( Such as buffer size, pointer availability, etc.,
when the check passes, the kernel takes the second data for actual
processing.

The function in that the patch modifies is _ctl_ioctl_main, I've cut
some details out of the function below to make my investigation more
readable.

,----
| 
| static long
| _ctl_ioctl_main(struct file *file, unsigned int cmd, void __user *arg,
| 	u8 compat, u16 mpi_version)
| {
| 	struct MPT3SAS_ADAPTER *ioc;
| 	struct mpt3_ioctl_header ioctl_header;
| 	enum block_state state;
| 	long ret = -EINVAL;
| 
| 	/* get IOCTL header */
| 	if (copy_from_user(&ioctl_header, (char __user *)arg,
| 	    sizeof(struct mpt3_ioctl_header))) {
| 		pr_err("failure at %s:%d/%s()!\n",
| 		    __FILE__, __LINE__, __func__);
| 		return -EFAULT;
| 	}
| 
`----

This is the first fetch, the priviledged user (root usually) enters this
area of code via an ioctl call on a /dev node.  It copies the userspace
memory, into the kernel structured defined here as n ioctl_header.


,----
| 
| if (_ctl_verify_adapter(ioctl_header.ioc_number,
| 			&ioc, mpi_version) == -1 || !ioc)
| 	return -ENODEV;
| 
`----

We then verify that the number passed in is a valid adapter on the
system..


next we're going to handle the different values that this ioctl can
take.

,----
| 
| -- snip -- 
| 
| 
| #ifdef CONFIG_COMPAT
| 	case MPT3COMMAND32:
| #endif
| 	case MPT3COMMAND:
| 	{
| 		struct mpt3_ioctl_command __user *uarg;
| 		struct mpt3_ioctl_command karg;
| 
| #ifdef CONFIG_COMPAT
| 		if (compat) {
| 			ret = _ctl_compat_mpt_command(ioc, cmd, arg);
| 			break;
| 		}
| #endif
| 		if (copy_from_user(&karg, arg, sizeof(karg))) {
| 			pr_err("failure at %s:%d/%s()!\n",
| 			    __FILE__, __LINE__, __func__);
| 			ret = -EFAULT;
| 			break;
| 		}
| 
`----


The copy_from_user above is the 'double fetch', which is usually not
great programming practice but there can be requirements for doing so,
for example in the MPT3COMMAND case the full ioctl_command structure may
be required.  But I digress..

,----
| 
| 	if (_IOC_SIZE(cmd) == sizeof(struct mpt3_ioctl_command)) {
| 		uarg = arg;
| 		ret = _ctl_do_mpt_command(ioc, karg, &uarg->mf);
| 	}
| 	break;
| }
| 
`----

In the above case, now there is a check, which if successfull, it drops
through into the _ctl_do_mpt_command function then immediately breaks
out of the case statement.

Because the break is unconditional, ie in the fetch situation it would
always exit, we can ignore the rest of the case statement for now.

,----
| 
| -- Snip -- 
| 
| out_unlock_pciaccess:
| 	mutex_unlock(&ioc->pci_access_mutex);
| 	return ret;
| }
| 
`----

So that means that this function is the only codepath where if the patch
fixes the problem,

Lets take a look at the proposed patch:


,----
| +		if (karg.hdr.ioc_number != ioctl_header.ioc_number) {
| +			ret = -EINVAL;
| +			break;
| +	
`----


If this is a fix, this would imply that somewhere in this codepath that
there is some use of this ioc_number, derived from karg.

The first copy copies -much- more than this, i had originally though
that it may have been a different condition that was double-fetched
however I dont see that either.

,----
| if (copy_from_user(&karg, arg, sizeof(karg))) {
|         pr_err("failure at %s:%d/%s()!\n",
|             __FILE__, __LINE__, __func__);
|         ret = -EFAULT;
|         break;
| }
`----

This copy from user copies a previously copied from user address into a
new range of memory, this range of memory is a struct
mpt3_ioctl_command.

So the obvious function where the attacker would be able to do work is
in the _ctl_do_mpt_command function, the data that they control is the
'karg' that is newly copied in from userspace.. a struct
mpt3_ioctl_command.

,----
| struct mpt3_ioctl_command {
|         struct mpt3_ioctl_header hdr;
|         uint32_t timeout;
|         void __user *reply_frame_buf_ptr;
|         void __user *data_in_buf_ptr;
|         void __user *data_out_buf_ptr;
|         void __user *sense_data_ptr;
|         uint32_t max_reply_bytes;
|         uint32_t data_in_size;
|         uint32_t data_out_size;
|         uint32_t max_sense_bytes;
|         uint32_t data_sge_offset;
|         uint8_t mf[1];
| };
| 
`----

Some of those might be nasty, so lets check _ctl_do_mpt_command, the
attacker can control or manipulate the second arg 'karg'

,----
| static long
| _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
|         void __user *mf)
| {
| 
| -- snip -- 
| 
| #### USE 1. - no use of ioc_numbere here.. cool.
| 
|         /* Check for overflow and wraparound */
|         if (karg.data_sge_offset * 4 > ioc->request_sz ||
|             karg.data_sge_offset > (UINT_MAX / 4)) {
|                 ret = -EINVAL;
|                 goto out;
|         }
| 
| -- Snip -- 
| 
| ### USE 2. - no use of ioc_number here..
| 
|         /* copy in request message frame from user */
|         if (copy_from_user(mpi_request, mf, karg.data_sge_offset*4)) {
|                 pr_err("failure at %s:%d/%s()!\n", __FILE__, __LINE__,
|                     __func__);
|                 ret = -EFAULT;
|                 goto out;
|         }
| 
| -- Snip --
| ### USE 3.: copying data based on the data_sge_offset, into yet another "request"
|         memcpy(request, mpi_request, karg.data_sge_offset*4);
| 
| -- Snip --
| ### USE 4.: copying the karg data out size and in size in, probably easier than typing many times.
| 
|         data_out_sz = karg.data_out_size;
|         data_in_sz = karg.data_in_size;
| 
| -- Snip -- 
| ### USE 5. Using the shorter abbreviation of data_out_sz 
| 
|                 if (copy_from_user(data_out, karg.data_out_buf_ptr,
|                         data_out_sz)) {
| 
| -- Snip --
| ### USE 6. psge math.. None of this relies on the ioc_number.
| 
|         psge = (void *)request + (karg.data_sge_offset*4);
| 
| -- Snip --
| ### USE 7. A NEW CHALLENGER APPEARS, lets check _ctl_set_task_mid
|            Need to check this function as it can reference any memory within 'karg' blob.
| 
|                         if (_ctl_set_task_mid(ioc, &karg, tm_request)) {
|                                 mpt3sas_base_free_smid(ioc, smid);
|                                 ioc->got_task_abort_from_ioctl = 0;
|                                 goto out;
|                         }
| 
| -- Snip --
| 
| Now inside _ctl_set_task_mid:
| 
| _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg, <-- KARG is the area containing ioc_number
|         Mpi2SCSITaskManagementRequest_t *tm_request)
| 
| -- Snip ---
| #### Use 7.1. min compare.. not relevant.
| 
|                 sz = min_t(u32, karg->max_reply_bytes, ioc->reply_sz);
| 
| -- Snip --
| #### Use 7.2.  Copy to user, based on "SZ", not touching or using the ioc_number
| 
|                 if (copy_to_user(karg->reply_frame_buf_ptr, ioc->ctl_cmds.reply,
|                     sz))
|                         pr_err("failure at %s:%d/%s()!\n", __FILE__,
|                             __LINE__, __func__);
| 
| -- snip --
| 
| #### END OF FUNCTION _ctl_set_task_mid
| #### Returning back to parent function: _ctl_do_mpt_command
| 
| ### Use 8. Sets the  function local timeout, no big deal.
| 
|         if (karg.timeout < MPT3_IOCTL_DEFAULT_TIMEOUT)
|                 timeout = MPT3_IOCTL_DEFAULT_TIMEOUT;
|         else
|                 timeout = karg.timeout;
| 
| -- Snip --
| 
| #### Use  9.  issuing a reset .
| 
|   -- not going to chase this because karg.data_sge_offset is passed
|   -- in as a parameter, and not karg as a blob.
| 
|                 issue_reset =
|                         mpt3sas_base_check_cmd_timeout(ioc,
|                                 ioc->ctl_cmds.status, mpi_request,
|                                 karg.data_sge_offset);
| 
| -- Skip --
| 
| #### Use 10. copy_to_user 
|      -- This is not an issue as it doesn't use the value we are toying with.
|      -- If there is a flaw here, its not he flaw i'm looking for.
| 
|         /* copy out xdata to user */
|         if (data_in_sz) {
|                 if (copy_to_user(karg.data_in_buf_ptr, data_in,
|                     data_in_sz)) {
|                         pr_err("failure at %s:%d/%s()!\n", __FILE__,
|                             __LINE__, __func__);
|                         ret = -ENODATA;
|                         goto out;
|                 }
|         }
| 
| -- Snip --
| 
| #### Use 11. the return of the check.
|      -- none of these concern themselves with this value.
| 
|         /* copy out reply message frame to user */
|         if (karg.max_reply_bytes) {
|                 sz = min_t(u32, karg.max_reply_bytes, ioc->reply_sz);
|                 if (copy_to_user(karg.reply_frame_buf_ptr, ioc->ctl_cmds.reply,
|                     sz)) {
|                         pr_err("failure at %s:%d/%s()!\n", __FILE__,
|                             __LINE__, __func__);
|                         ret = -ENODATA;
|                         goto out;
|                 }
|         }
| 
| 
| -- snip ---
| 
| #### Use 12. Error handling mostly...
| 
|   -- error handling, no functions called.
|   -- checks for sense bytes.
|   -- checks for sense_data-
|   -- copies to user based on karg.sense_data_ptr dont think that this is exploitable or relevant.
| 
|         /* copy out sense/NVMe Error Response to user */
|         if (karg.max_sense_bytes && (mpi_request->Function == 
|             MPI2_FUNCTION_SCSI_IO_REQUEST || mpi_request->Function ==
|             MPI2_FUNCTION_RAID_SCSI_IO_PASSTHROUGH || mpi_request->Function ==
|             MPI2_FUNCTION_NVME_ENCAPSULATED)) {
|                 if (karg.sense_data_ptr == NULL) {
|                         ioc_info(ioc, "Response buffer provided by application is NULL; Response data will not be returned\n");
|                         goto out;
|                 }
|                 sz_arg = (mpi_request->Function ==
|                 MPI2_FUNCTION_NVME_ENCAPSULATED) ? NVME_ERROR_RESPONSE_SIZE :
|                                                         SCSI_SENSE_BUFFERSIZE;
|                 sz = min_t(u32, karg.max_sense_bytes, sz_arg);
|                 if (copy_to_user(karg.sense_data_ptr, ioc->ctl_cmds.sense,
|                     sz)) {
|                         pr_err("failure at %s:%d/%s()!\n", __FILE__,
|                                 __LINE__, __func__);
|                         ret = -ENODATA;
|                         goto out;
|                 }
|         }
| 
`----

The function returns back to userspace, and I dont' see where this patch would have solved the problem as the ioc_number value is not used in this codepath.


Note You need to log in before you can comment on or make changes to this bug.