| Message ID | 20220916224741.2269649-1-tadeusz.struk@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: mon: make mmapped memory read only | expand |
On 9/16/22 15:47, Tadeusz Struk wrote: > Syzbot found an issue in usbmon where it can corrupt monitor > internal memory causing the usbmon to crash with segfault, > UAF, etc. The reproducer mmaps the /dev/usbmon memory to userspace > and overwrites it with arbitrary data, which causes the issues. > To prevent that explicitly clear the VM_WRITE flag in mon_bin_mmap(). > > Cc:linux-usb@vger.kernel.org > Cc:linux-kernel@vger.kernel.org > Cc:stable@vger.kernel.org > Fixes: 6f23ee1fefdc ("USB: add binary API to usbmon") > Link:https://syzkaller.appspot.com/bug?id=2eb1f35d6525fa4a74d75b4244971e5b1411c95a > Signed-off-by: Tadeusz Struk<tadeusz.struk@linaro.org> I forgot to add: Reported-by: syzbot+23f57c5ae902429285d7@syzkaller.appspotmail.com
Hi Tadeusz, Looking at places like these: https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/infiniband/hw/qib/qib_file_ops.c#L736 https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/infiniband/hw/mlx5/main.c#L2088 I think we also need to remove VM_MAYWRITE, otherwise it's still possible to turn it into a writable mapping with mprotect. It's also probably better to return an error if VM_WRITE (or VM_EXEC?) is set rather than silently fix it up.
On Mon, 19 Sept 2022 at 06:25, Dmitry Vyukov <dvyukov@google.com> wrote: > > Hi Tadeusz, > > Looking at places like these: > https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/infiniband/hw/qib/qib_file_ops.c#L736 > https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/infiniband/hw/mlx5/main.c#L2088 > I think we also need to remove VM_MAYWRITE, otherwise it's still > possible to turn it into a writable mapping with mprotect. > > It's also probably better to return an error if VM_WRITE (or VM_EXEC?) is set > rather than silently fix it up. The credit for the VM_MAYWRITE suggestion goes to the PaX Team. Suggested-by: PaX Team <pageexec@freemail.hu>
Hi Dmitry, On 9/18/22 21:25, Dmitry Vyukov wrote: > Hi Tadeusz, > > Looking at places like these: > https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/infiniband/hw/qib/qib_file_ops.c#L736 > https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/infiniband/hw/mlx5/main.c#L2088 > I think we also need to remove VM_MAYWRITE, otherwise it's still > possible to turn it into a writable mapping with mprotect. > > It's also probably better to return an error if VM_WRITE (or VM_EXEC?) is set > rather than silently fix it up. Yes, I think that returning an error will make more sense here. I don't think we need to worry about VM_EXEC. Even if userspace will try to execute from the mmaped location it won't work. It will probably crash the application without causing any harm to the kernel. I will update the patch and send a v2 soon.
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c index f48a23adbc35..f452fc03093c 100644 --- a/drivers/usb/mon/mon_bin.c +++ b/drivers/usb/mon/mon_bin.c @@ -1268,6 +1268,7 @@ static int mon_bin_mmap(struct file *filp, struct vm_area_struct *vma) { /* don't do anything here: "fault" will set up page table entries */ vma->vm_ops = &mon_bin_vm_ops; + vma->vm_flags &= ~VM_WRITE; vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP; vma->vm_private_data = filp->private_data; mon_bin_vma_open(vma);
Syzbot found an issue in usbmon where it can corrupt monitor internal memory causing the usbmon to crash with segfault, UAF, etc. The reproducer mmaps the /dev/usbmon memory to userspace and overwrites it with arbitrary data, which causes the issues. To prevent that explicitly clear the VM_WRITE flag in mon_bin_mmap(). Cc: linux-usb@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org Fixes: 6f23ee1fefdc ("USB: add binary API to usbmon") Link: https://syzkaller.appspot.com/bug?id=2eb1f35d6525fa4a74d75b4244971e5b1411c95a Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org> --- drivers/usb/mon/mon_bin.c | 1 + 1 file changed, 1 insertion(+)