cam_periph_(un)mapmem issues with XPT_MMC_IO

From: Andriy Gapon <avg_at_FreeBSD.org>
Date: Sat, 06 Nov 2021 16:33:08 UTC
I think that I see a few issues with cam_periph_(un)mapmem handling of 
XPT_MMC_IO ccbs.

First, I think that this piece of code sets the length incorrectly:
                 data_ptrs[0] = (unsigned char **)&ccb->mmcio.cmd.data;
                 lengths[0] = sizeof(struct mmc_data *);
I think that it should be sizeof(struct mmc_data) as it seems that we want to 
map the whole structure into the kernel memory.

Then, I think that this code is not safe / correct:
                 data_ptrs[1] = (unsigned char **)&ccb->mmcio.cmd.data->data;
                 lengths[1] = ccb->mmcio.cmd.data->len;
I believe that the code accesses internals of ccb->mmcio.cmd.data via a userland 
pointer as that object is not mapped into the kernel address space yet and the 
pointer is not adjusted yet.

Finally, I think that there is a problem with unmapping of those two data 
buffers.  In all other cases where cam_periph_unmapmem() works on multiple 
memory locations (numbufs > 2), those locations are independent of each other.
But for XPT_MMC_IO one buffer contains a pointer to other buffer, so there is a 
dependency between them.

It seems that there is an access to mmcio.cmd.data object via its kernel pointer 
after the object is unmapped from the kernel space because it comes before 
mmcio.cmd.data->data in the array.

Running a command like
# camcontrol mmcsdcmd 2:0:0 -c 8 -a 0 -f 0x35 -l 512

I get the following crash (on arm):
panic: vm_fault_lookup: fault on nofault entry, addr: 0xde367000
cpuid = 2
time = 1636189704
KDB: stack backtrace:
db_trace_self() at db_trace_self
db_trace_self_wrapper() at db_trace_self_wrapper+0x30
vpanic() at vpanic+0x17c
doadump() at doadump
vm_fault() at vm_fault+0x17b0
vm_fault_trap() at vm_fault_trap+0x78
abort_handler() at abort_handler+0x3c8
exception_exit() at exception_exit
cam_periph_unmapmem() at cam_periph_unmapmem+0x2e4
passsendccb() at passsendccb+0x194
passdoioctl() at passdoioctl+0xcc
passioctl() at passioctl+0x28
devfs_ioctl() at devfs_ioctl+0xcc
vn_ioctl() at vn_ioctl+0x12c
devfs_ioctl_f() at devfs_ioctl_f+0x2c
kern_ioctl() at kern_ioctl+0x318
sys_ioctl() at sys_ioctl+0x108

Unfortunately I do not have a crash dump, only a serial console output.
As far as I can tell cam_periph_unmapmem+0x2e4 corresponds to the assignment in 
the following code:
                 /* Set the user's pointer back to the original value */
                 *data_ptrs[i] = mapinfo->orig[i];

As a quick hack I tried to reverse the order of iteration and it seems to have 
fixed the crash.

In general, it seems that cam_periph_(un)mapmem is not good for the MMC case 
because of the dependency between buffers.  Perhaps the code could be extended 
to handle dependent buffers.  Or maybe MMC should get its own special routines 
for mapping and unmapping the buffers.

Warner and Alexander, I Bcc-ed you for r320844 /
a94a63f0a6bc1 and r345656 / b059686a71c89.  One added XPT_MMC_IO to 
cam_periph_mapmem and the other added XPT_MMC_IO to cam_periph_unmapmem along 
with multitude of other changes.

-- 
Andriy Gapon