Skip to content

Commit

Permalink
FIXME: possible leak of kernel info in processing user params
Browse files Browse the repository at this point in the history
All callers of smith_copy_from_user and smith_get_user are
vulnerable since these 2 functions don't validate the user
parameters (could be addresses in kernel space) which could
be easily manipulated by user apps, eg in write_pre_handler.

The risk is low, since root privilege is required to access
our HIDS log.

Signed-off-by: shenping.matt <shenping.matt@bytedance.com>
  • Loading branch information
shenping-bd committed Jun 15, 2023
1 parent 53ffcbf commit cd177ce
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 15 deletions.
6 changes: 6 additions & 0 deletions driver/LKM/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ ifneq ($(IPV6_SUPPORT),)
ccflags-y += -D IPV6_SUPPORT
endif

UACCESS_FILES := $(shell find -L $(K_I_PATH) -path \*/asm-generic/uaccess.h) /dev/null
UACCESS_SUPPORT := $(shell sh -c "grep -sE define[[:space:]]\+access_ok $(UACCESS_FILES) | grep type")
ifneq ($(UACCESS_SUPPORT),)
ccflags-y += -D UACCESS_TYPE_SUPPORT
endif

TRACE_EVENTS_HEADER := /lib/modules/$(KERNEL_HEAD)/build/include/linux/trace_events.h
TRACE_EVENTS_HEADER_V := $(TRACE_EVENTS_HEADER)
TRACE_EVENTS_HEADER_CHECK := $(shell test -e $(TRACE_EVENTS_HEADER_V))
Expand Down
51 changes: 36 additions & 15 deletions driver/LKM/include/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -226,15 +226,50 @@ static __always_inline long __must_check smith_strnlen_user(const char __user *s
return res;
}

/*
* WARNING:
*
* access_ok() might sleep as it 's said, but actaully what it does
* is just a comparison between user addr and current's TASK_SIZE_MAX.
*/
static __always_inline int smith_access_ok(const void __user *from, unsigned long n)
{
#if defined(UACCESS_TYPE_SUPPORT)
return access_ok(VERIFY_READ, from, n);
#else
return access_ok(from, n);
#endif
}

static __always_inline unsigned long __must_check smith_copy_from_user(void *to, const void __user *from, unsigned long n)
{
unsigned long res;
smith_pagefault_disable();
res = __copy_from_user_inatomic(to, from, n);
/* validate user-mode buffer: ['from' - 'from' + 'n') */
if (smith_access_ok(from, n))
res = __copy_from_user_inatomic(to, from, n);
else
res = n;
smith_pagefault_enable();
return res;
}

/* get_user() will call might_fault(), which violates
the rules of atomic context (introdcued by kprobe) */
#define smith_get_user(x, ptr) \
({ \
unsigned long __val = 0; \
int __ret; \
smith_pagefault_disable(); \
/* validate user-mode buffer: ['from' - 'from' + 'n') */ \
__ret = sizeof(*(ptr)); \
if (smith_access_ok(ptr, __ret)) \
__ret = __copy_from_user_inatomic(&__val, ptr, __ret); \
smith_pagefault_enable(); \
(x) = (__typeof__(*(ptr)))__val; \
__ret; \
})

static __always_inline char *smith_d_path(const struct path *path, char *buf, int buflen)
{
char *name = DEFAULT_RET_STR;
Expand Down Expand Up @@ -309,20 +344,6 @@ static __always_inline char *smith_get_exe_file(char *buffer, int size)
return exe_file_str;
}

/* get_user() will call might_fault(), which violates
the rules of atomic context (introdcued by kprobe) */
#define smith_get_user(x, ptr) \
({ \
unsigned long __val = 0; \
int __ret; \
smith_pagefault_disable(); \
__ret = __copy_from_user_inatomic(&__val, ptr, \
sizeof(*(ptr))); \
smith_pagefault_enable(); \
(x) = (__typeof__(*(ptr)))__val; \
__ret; \
})

static inline void *__get_dns_query(unsigned char *data, int index, char *res) {
int i;
int flag = -1;
Expand Down

0 comments on commit cd177ce

Please sign in to comment.