Bug 1717182 (CVE-2019-12456)

Created kernel tracking bugs for this issue:

Affects: fedora-all [bug 1717183]

Initial investigation of this flaw does not conclude that this flaw is exploitable.  Upstream discussion on this flaw will continue.

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.