Archive-Team
/
zfs
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases 103 Wiki Activity

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
This commit is contained in:
Chunwei Chen 2018-03-12 11:27:02 -07:00 committed by Brian Behlendorf
parent b7eec00f9f
commit 9470cbd4f9
2 changed files with 12 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
include/sys/trace_zrlock.h
View File

@ -42,27 +42,27 @@
*/ */
/* BEGIN CSTYLED */ /* BEGIN CSTYLED */
DECLARE_EVENT_CLASS(zfs_zrlock_class, DECLARE_EVENT_CLASS(zfs_zrlock_class,
TP_PROTO(zrlock_t *zrl, uint32_t n), TP_PROTO(zrlock_t *zrl, kthread_t *owner, uint32_t n),
TP_ARGS(zrl, n), TP_ARGS(zrl, owner, n),
TP_STRUCT__entry( TP_STRUCT__entry(
__field(int32_t, refcount) __field(int32_t, refcount)
#ifdef ZFS_DEBUG #ifdef ZFS_DEBUG
__field(pid_t, owner_pid) __field(pid_t, owner_pid)
__string(caller, zrl->zr_caller) __field(const char *, caller)
#endif #endif
__field(uint32_t, n) __field(uint32_t, n)
), ),
TP_fast_assign( TP_fast_assign(
__entry->refcount = zrl->zr_refcount; __entry->refcount = zrl->zr_refcount;
#ifdef ZFS_DEBUG #ifdef ZFS_DEBUG
__entry->owner_pid = zrl->zr_owner ? zrl->zr_owner->pid : 0; __entry->owner_pid = owner ? owner->pid : 0;
__assign_str(caller, zrl->zr_caller); __entry->caller = zrl->zr_caller ? zrl->zr_caller : "(null)";
#endif #endif
__entry->n = n; __entry->n = n;
), ),
#ifdef ZFS_DEBUG #ifdef ZFS_DEBUG
TP_printk("zrl { refcount %d owner_pid %d caller %s } n %u", TP_printk("zrl { refcount %d owner_pid %d caller %s } n %u",
__entry->refcount, __entry->owner_pid, __get_str(caller), __entry->refcount, __entry->owner_pid, __entry->caller,
__entry->n) __entry->n)
#else #else
TP_printk("zrl { refcount %d } n %u", TP_printk("zrl { refcount %d } n %u",
@ -73,8 +73,8 @@ DECLARE_EVENT_CLASS(zfs_zrlock_class,
#define DEFINE_ZRLOCK_EVENT(name) \ #define DEFINE_ZRLOCK_EVENT(name) \
DEFINE_EVENT(zfs_zrlock_class, name, \ DEFINE_EVENT(zfs_zrlock_class, name, \
TP_PROTO(zrlock_t *zrl, uint32_t n), \ TP_PROTO(zrlock_t *zrl, kthread_t *owner, uint32_t n), \
TP_ARGS(zrl, n)) TP_ARGS(zrl, owner, n))
DEFINE_ZRLOCK_EVENT(zfs_zrlock__reentry); DEFINE_ZRLOCK_EVENT(zfs_zrlock__reentry);
#endif /* _TRACE_ZRLOCK_H */ #endif /* _TRACE_ZRLOCK_H */

6
module/zfs/zrlock.c
View File

@ -82,8 +82,10 @@ zrl_add_impl(zrlock_t *zrl, const char *zc)
ASSERT3S((int32_t)n, >=, 0); ASSERT3S((int32_t)n, >=, 0);
#ifdef ZFS_DEBUG #ifdef ZFS_DEBUG
if (zrl->zr_owner == curthread) { if (zrl->zr_owner == curthread) {
DTRACE_PROBE2(zrlock__reentry, DTRACE_PROBE3(zrlock__reentry,
zrlock_t *, zrl, uint32_t, n); zrlock_t *, zrl,
kthread_t *, curthread,
uint32_t, n);
} }
zrl->zr_owner = curthread; zrl->zr_owner = curthread;
zrl->zr_caller = zc; zrl->zr_caller = zc;