Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

make it clear that UAFs are UAFs in reports #56

Closed
dvyukov opened this issue Aug 10, 2019 · 7 comments
Closed

make it clear that UAFs are UAFs in reports #56

dvyukov opened this issue Aug 10, 2019 · 7 comments
Assignees
@ramosian-glider

Comments

@dvyukov
Copy link
Collaborator

dvyukov commented Aug 10, 2019

Currently for UAFs we produce a normal uninit-value report with an origin in kfree. This is somewhat confusing. It can make sense to explicitly say in such reports that this is a UAF.
We discussed that this can be done by marking such origin with a dedicated bit (we seem to have spare bits in origins). Then we could check that the initial origin has this bit set to detect UAFs.
Also, the bit check is cheap, so we potentially could check it right in the loads and produce UAF reports earlier.
@ramosian-glider ramosian-glider self-assigned this Oct 1, 2019
@dvyukov
Copy link
Collaborator Author

dvyukov commented Oct 4, 2019

Up. We are getting more UAFs from KMSAN which turn out to be dups of KASAN reported bugs.

@ramosian-glider
Copy link
Member

This is on my radar. I have a draft patch for it.

@ramosian-glider
Copy link
Member

We do report UAFs now, but not in the loads yet.

@ramosian-glider
Copy link
Member

It is also theoretically possible to report UAFs in stores as well. But doing so is a bit of a hassle, as SLUB performs numerous stores to freed memory, which require careful handling (we don't want to report them).

@ramosian-glider
Copy link
Member

322dad0 implements UAF detection on each load.
Let's see how it goes.

@dvyukov
Copy link
Collaborator Author

dvyukov commented Nov 27, 2019

Does this add slowdown (and code) for something we don't want to use?

@ramosian-glider
Copy link
Member

We've decided to put all UAF bugs into a single bucket in syzkaller anyway, so I'd better revert this improved UAF detection for now (as it costs us some performance).
We can revisit this issue if upstream developers request more precise UAF detection.

ramosian-glider pushed a commit that referenced this issue Jan 8, 2020
@stefano-garzarella @davem330
vsock/virtio: fix null-pointer dereference in virtio_transport_recv_l…
df18fa1 
…isten()

With multi-transport support, listener sockets are not bound to any
transport. So, calling virtio_transport_reset(), when an error
occurs, on a listener socket produces the following null-pointer
dereference:

  BUG: kernel NULL pointer dereference, address: 00000000000000e8
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  PGD 0 P4D 0
  Oops: 0000 [#1] SMP PTI
  CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.5.0-rc1-ste-00003-gb4be21f316ac-dirty #56
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
  Workqueue: virtio_vsock virtio_transport_rx_work [vmw_vsock_virtio_transport]
  RIP: 0010:virtio_transport_send_pkt_info+0x20/0x130 [vmw_vsock_virtio_transport_common]
  Code: 1f 84 00 00 00 00 00 0f 1f 00 55 48 89 e5 41 57 41 56 41 55 49 89 f5 41 54 49 89 fc 53 48 83 ec 10 44 8b 76 20 e8 c0 ba fe ff <48> 8b 80 e8 00 00 00 e8 64 e3 7d c1 45 8b 45 00 41 8b 8c 24 d4 02
  RSP: 0018:ffffc900000b7d08 EFLAGS: 00010282
  RAX: 0000000000000000 RBX: ffff88807bf12728 RCX: 0000000000000000
  RDX: ffff88807bf12700 RSI: ffffc900000b7d50 RDI: ffff888035c84000
  RBP: ffffc900000b7d40 R08: ffff888035c84000 R09: ffffc900000b7d08
  R10: ffff8880781de800 R11: 0000000000000018 R12: ffff888035c84000
  R13: ffffc900000b7d50 R14: 0000000000000000 R15: ffff88807bf12724
  FS:  0000000000000000(0000) GS:ffff88807dc00000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 00000000000000e8 CR3: 00000000790f4004 CR4: 0000000000160ef0
  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  Call Trace:
   virtio_transport_reset+0x59/0x70 [vmw_vsock_virtio_transport_common]
   virtio_transport_recv_pkt+0x5bb/0xe50 [vmw_vsock_virtio_transport_common]
   ? detach_buf_split+0xf1/0x130
   virtio_transport_rx_work+0xba/0x130 [vmw_vsock_virtio_transport]
   process_one_work+0x1c0/0x300
   worker_thread+0x45/0x3c0
   kthread+0xfc/0x130
   ? current_work+0x40/0x40
   ? kthread_park+0x90/0x90
   ret_from_fork+0x35/0x40
  Modules linked in: sunrpc kvm_intel kvm vmw_vsock_virtio_transport vmw_vsock_virtio_transport_common irqbypass vsock virtio_rng rng_core
  CR2: 00000000000000e8
  ---[ end trace e75400e2ea2fa824 ]---

This happens because virtio_transport_reset() calls
virtio_transport_send_pkt_info() that can be used only on
connecting/connected sockets.

This patch fixes the issue, using virtio_transport_reset_no_sock()
instead of virtio_transport_reset() when we are handling a listener
socket.

Fixes: c0cfa2d ("vsock: add multi-transports support")
Signed-off-by: Stefano Garzarella <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ramosian-glider ramosian-glider

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ramosian-glider @dvyukov