]> git.proxmox.com Git - mirror_zfs.git/commit - module/zfs/zrlock.c
Fix race in trace point in zrl_add_impl
authorChunwei Chen <tuxoko@gmail.com>
Mon, 12 Mar 2018 18:27:02 +0000 (11:27 -0700)
committerBrian Behlendorf <behlendorf1@llnl.gov>
Mon, 12 Mar 2018 18:27:02 +0000 (11:27 -0700)
commit9470cbd4f95ecd730d18f7dadd56bcf54c6711b0
treec6088ab0720a1812380e938a86fa80077d1dccd0
parentb7eec00f9fc05e883b0dadb231251b5996d02e3e
Fix race in trace point in zrl_add_impl

We hit an illegal memory access in the zrlock trace point. The problem
is that zrl->zr_owner and zrl->zr_caller are assigned locklessly. And if
zrl->zr_owner got assigned a longer string between when __string()
calculate the strlen, and when __assign_str() does strcpy. The copy will
overflow the buffer.

==
For example:

Initial condition:
zrl->zr_owner = A
zrl->zr_caller = "abc"

Thread A                                 Thread B
-------------------------------------------------
if (zrl->zr_owner == A) {
  DTRACE_PROBE2() {
    __string() {
      strlen(zrl->zr_caller) -> 3
      allocate buf[4]
    }

                                        zrl->zr_owner = B
        zrl->zr_caller = "abcd"

    __assign_str() {
      strcpy(buf, zrl->zr_caller) <- buffer overflow
==

Dereferencing zrl->zr_owner->pid may also be problematic, in that the
zrl->zr_owner got changed to other task, and that task exits, freeing
the task_struct. This should be very unlikely, as the other task need to
zrl_remove and exit between the dereferencing zr->zr_owner and
zr->zr_owner->pid. Nevertheless, we'll deal with it as well.

To fix the zrl->zr_caller issue, instead of copy the string content, we
just copy the pointer, this is safe because it always points to
__func__, which is static. As for the zrl->zr_owner issue, we pass in
curthread instead of using zrl->zr_owner.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Chunwei Chen <david.chen@nutanix.com>
Closes #7291
include/sys/trace_zrlock.h
module/zfs/zrlock.c