Re: cam_periph_(un)mapmem issues with XPT_MMC_IO
- In reply to: Andriy Gapon : "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:43:59 UTC
Hi Andriy, I agree with your analysis. My opinion is that your second suggestion is preferable, the mmc module should gets its own custom handlers. Thanks, Scott > On Nov 6, 2021, at 10:33 AM, Andriy Gapon <avg@freebsd.org> wrote: > > > 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 >