Re: [LTP] [PATCH] epoll_pwait04: Use raw syscalls to avoid SIGSEGV on 32-bit Bionic

[Petr Vorel <pvorel@suse.cz>]

Hi Wake Liu, all,

> epoll_pwait04 passes an unmapped address from tst_get_bad_addr() as the
> sigmask argument to verify that the kernel returns EFAULT. However,
> do_epoll_pwait() invokes the libc epoll_pwait() and epoll_pwait2()
> wrappers when available.

> On 32-bit Android Bionic, sizeof(sigset_t) is 4 bytes whereas the
> kernel expects an 8-byte (_NSIG / 8) signal set. The 32-bit Bionic
> wrappers therefore dereference the sigmask pointer in userspace to
> zero-extend it into a sigset64_t on the stack before issuing the
> syscall. Passing an unmapped bad_addr thus causes a SIGSEGV in
> userspace before reaching the kernel. As passing an invalid pointer
> to a libc function is undefined behavior anyway, the EFAULT check
> should call the syscalls directly.

LGTM, thank you.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> Expose sys_epoll_pwait() and sys_epoll_pwait2() in lapi/epoll.h and add
> do_sys_epoll_pwait() in epoll_pwait_var.h so epoll_pwait04 invokes the
> raw syscalls directly. do_epoll_pwait() is made static inline to avoid
> a -Wunused-function warning now that epoll_pwait04 no longer uses it.

Yeah, IMHO it's mostly better to use 'static inline' for headers (FYI for
library code we often move them to C files, but we do not put syscall
specific helpers into LTP library).

Kind regards,
Petr

> Fixes: 9d083e3a286b ("epoll_pwait: Add invalid sigmask test for epoll_pwait04")
> Signed-off-by: Wake Liu <wakel@google.com>
> ---
>  include/lapi/epoll.h                          | 37 ++++++++++++++-----
>  .../syscalls/epoll_pwait/epoll_pwait04.c      |  6 ++-
>  .../syscalls/epoll_pwait/epoll_pwait_var.h    | 30 +++++++++++----
>  3 files changed, 55 insertions(+), 18 deletions(-)

> diff --git a/include/lapi/epoll.h b/include/lapi/epoll.h
> index 15a954175..07fb1503a 100644
> --- a/include/lapi/epoll.h
> +++ b/include/lapi/epoll.h
> @@ -24,13 +24,24 @@ static inline void epoll_pwait_supported(void)
>  	tst_syscall(__NR_epoll_pwait);
>  }

> +/*
> + * Note: The kernel expects an _NSIG / 8 bytes signal set, which may be larger
> + * than sizeof(sigset_t) on some 32-bit libcs (e.g. 4 bytes on 32-bit Bionic).
> + */
> +static inline int sys_epoll_pwait(int epfd, struct epoll_event *events,
> +				  int maxevents, int timeout,
> +				  const sigset_t *sigmask)
> +{
> +	return tst_syscall(__NR_epoll_pwait, epfd, events, maxevents,
> +			   timeout, sigmask, _NSIG / 8);
> +}
> +
>  #ifndef HAVE_EPOLL_PWAIT
>  static inline int epoll_pwait(int epfd, struct epoll_event *events,
>  			      int maxevents, int timeout,
>  			      const sigset_t *sigmask)
>  {
> -	return tst_syscall(__NR_epoll_pwait, epfd, events, maxevents,
> -			   timeout, sigmask, _NSIG / 8);
> +	return sys_epoll_pwait(epfd, events, maxevents, timeout, sigmask);
>  }
>  #endif

> @@ -40,23 +51,31 @@ static inline void epoll_pwait2_supported(void)
>  	tst_syscall(__NR_epoll_pwait2);
>  }

> -#ifndef HAVE_EPOLL_PWAIT2
> -static inline int epoll_pwait2(int epfd, struct epoll_event *events,
> -			       int maxevents, const struct timespec *timeout,
> -			       const sigset_t *sigmask)
> +static inline int sys_epoll_pwait2(int epfd, struct epoll_event *events,
> +				   int maxevents,
> +				   const struct timespec *timeout,
> +				   const sigset_t *sigmask)
>  {
> -	if (timeout == NULL)
> +	struct __kernel_timespec ts;
> +
> +	if (!timeout)
>  		return tst_syscall(__NR_epoll_pwait2, epfd, events, maxevents,
>  				   NULL, sigmask, _NSIG / 8);

> -	struct __kernel_timespec ts;
> -
>  	ts.tv_sec = timeout->tv_sec;
>  	ts.tv_nsec = timeout->tv_nsec;

>  	return tst_syscall(__NR_epoll_pwait2, epfd, events, maxevents,
>  			   &ts, sigmask, _NSIG / 8);
>  }
> +
> +#ifndef HAVE_EPOLL_PWAIT2
> +static inline int epoll_pwait2(int epfd, struct epoll_event *events,
> +			       int maxevents, const struct timespec *timeout,
> +			       const sigset_t *sigmask)
> +{
> +	return sys_epoll_pwait2(epfd, events, maxevents, timeout, sigmask);
> +}
>  #endif

>  #endif /* LAPI_EPOLL_H__ */
> diff --git a/testcases/kernel/syscalls/epoll_pwait/epoll_pwait04.c b/testcases/kernel/syscalls/epoll_pwait/epoll_pwait04.c
> index b399225b9..4b77c3bd0 100644
> --- a/testcases/kernel/syscalls/epoll_pwait/epoll_pwait04.c
> +++ b/testcases/kernel/syscalls/epoll_pwait/epoll_pwait04.c
> @@ -20,7 +20,11 @@ static void *bad_addr;

>  static void run(void)
>  {
> -	TST_EXP_FAIL(do_epoll_pwait(efd, &e, 1, NULL, bad_addr),
> +	/*
> +	 * Use raw syscalls, libc wrappers may dereference sigmask in
> +	 * userspace (e.g. 32-bit Bionic) and crash with SIGSEGV.
> +	 */
> +	TST_EXP_FAIL(do_sys_epoll_pwait(efd, &e, 1, NULL, bad_addr),
>  		     EFAULT, "with an invalid sigmask pointer");
>  }

> diff --git a/testcases/kernel/syscalls/epoll_pwait/epoll_pwait_var.h b/testcases/kernel/syscalls/epoll_pwait/epoll_pwait_var.h
> index e215a6540..07d2ac8ca 100644
> --- a/testcases/kernel/syscalls/epoll_pwait/epoll_pwait_var.h
> +++ b/testcases/kernel/syscalls/epoll_pwait/epoll_pwait_var.h
> @@ -13,21 +13,35 @@
>  #define MSEC_PER_SEC (1000L)
>  #define NSEC_PER_MSEC (1000000L)

> -static int do_epoll_pwait(int epfd, struct epoll_event *events, int
> -	maxevents, struct timespec *timeout, const sigset_t *sigmask)
> +static inline int epoll_timeout_ms(const struct timespec *timeout)
> +{
> +	if (!timeout)
> +		return -1;
> +
> +	return timeout->tv_sec * MSEC_PER_SEC +
> +		(timeout->tv_nsec + NSEC_PER_MSEC - 1) / NSEC_PER_MSEC;
> +}
> +
> +static inline int do_epoll_pwait(int epfd, struct epoll_event *events,
> +	int maxevents, struct timespec *timeout, const sigset_t *sigmask)
>  {
>  	if (tst_variant == 1)
>  		return epoll_pwait2(epfd, events, maxevents, timeout, sigmask);

> -	int timeout_ms = -1;
> +	return epoll_pwait(epfd, events, maxevents, epoll_timeout_ms(timeout),
> +			   sigmask);
> +}

> -	if (timeout) {
> -		timeout_ms = timeout->tv_sec * MSEC_PER_SEC;
> -		timeout_ms += (timeout->tv_nsec + NSEC_PER_MSEC - 1) /
> -			NSEC_PER_MSEC;
> +static inline int do_sys_epoll_pwait(int epfd, struct epoll_event *events,
> +	int maxevents, struct timespec *timeout, const sigset_t *sigmask)
> +{
> +	if (tst_variant == 1) {
> +		return sys_epoll_pwait2(epfd, events, maxevents, timeout,
> +					sigmask);
>  	}

> -	return epoll_pwait(epfd, events, maxevents, timeout_ms, sigmask);
> +	return sys_epoll_pwait(epfd, events, maxevents,
> +			       epoll_timeout_ms(timeout), sigmask);
>  }

>  static void epoll_pwait_init(void)

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp