cam_periph_(un)mapmem issues with XPT_MMC_IO
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
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