[LTP] [PATCH v5 1/3] syscalls/modify_ldt: Add lapi/ldt.h
Petr Vorel
pvorel@suse.cz
Tue Apr 1 10:48:27 CEST 2025
Hi Ricardo,
> Add a wrapper to modify_ldt which is used in a few tests and should be
> reused.
Generally LGTM. Few notes below.
> Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.com>
> Signed-off-by: Ricardo B. Marlière <rbm@suse.com>
> ---
> configure.ac | 1 -
> include/lapi/ldt.h | 34 ++++++++++++++++++++++++++++++
> testcases/cve/cve-2015-3290.c | 37 +++++++--------------------------
> testcases/cve/cve-2017-17053.c | 10 ++++-----
> testcases/kernel/syscalls/fork/fork05.c | 5 ++---
> 5 files changed, 48 insertions(+), 39 deletions(-)
> diff --git a/configure.ac b/configure.ac
> index 671079f3aa48c72866f69e7c545d3428ba87f931..b22f3eacdb1ccb764eae443ab1fb70afd971a14c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -46,7 +46,6 @@ AC_CHECK_DECLS([PR_CAPBSET_DROP, PR_CAPBSET_READ],,,[#include <sys/prctl.h>])
> AC_CHECK_DECLS([SEM_STAT_ANY],,,[#include <sys/sem.h>])
> AC_CHECK_HEADERS_ONCE([ \
> - asm/ldt.h \
I guess because <asm/ldt.h> is old enough (was here before UAPI split in kernel
v3.8-rc1 we can remove it. But it'd be nice to mention the reason in the commit
message (technically it's a separate change, ok to do that in the commit while
you're at it, but mentioning the reason helps in case of regression).
BTW it'd be nice to go over our m4/ and configure.ac and drop things old enough
to speedup configure run. AC_CHECK_HEADERS_ONCE() is probably quite quick, m4/
files probably take more time. But every speedup counts.
FYI we support quite old distros: kernel 4.4 (in SLE12-SP2):
https://linux-test-project.readthedocs.io/en/latest/users/supported_systems.html
> asm/prctl.h \
> cpuid.h \
> emmintrin.h \
> diff --git a/include/lapi/ldt.h b/include/lapi/ldt.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..bab5404457b959a5b44adbf6f458602c5121dbb8
> --- /dev/null
> +++ b/include/lapi/ldt.h
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2025 SUSE LLC Ricardo B. Marlière <rbm@suse.com>
> + */
> +
> +#ifndef LAPI_LDT_H__
> +#define LAPI_LDT_H__
> +
> +#include "config.h"
> +#include "lapi/syscalls.h"
> +#include <asm/ldt.h>
> +
> +static int modify_ldt(int func, void *ptr, unsigned long bytecount)
> +{
> + return tst_syscall(__NR_modify_ldt, func, ptr, bytecount);
> +}
> +
> +static int safe_modify_ldt(const char *file, const int lineno, int func,
> + void *ptr, unsigned long bytecount)
> +{
> + int rval;
> +
> + rval = modify_ldt(func, ptr, bytecount);
> + if (rval == -1)
> + tst_brk_(file, lineno, TBROK | TERRNO,
> + "modify_ldt(%d, %p, %lu)", func, ptr, bytecount);
> +
> + return rval;
> +}
You dropped 'const' from struct user_desc data_desc definitions.
Using const struct user_desc *ptr as a parameter would help to keep the
function. Maybe I'm wrong but I guess if there were libc wrapper (no libc has
it) it would prefer to have pointer to struct than plain void as a parameter.
static int modify_ldt(int func, const struct user_desc *ptr, unsigned long bytecount)
{
return tst_syscall(__NR_modify_ldt, func, ptr, bytecount);
}
static int safe_modify_ldt(const char *file, const int lineno, int func,
const struct user_desc *ptr, unsigned long bytecount)
{
int rval;
rval = modify_ldt(func, ptr, bytecount);
if (rval == -1)
tst_brk_(file, lineno, TBROK | TERRNO,
"modify_ldt(%d, %p, %lu)", func, &ptr, bytecount);
return rval;
}
nit: IMHO modify_ldt() is not used anywhere outside lapi file, right?
I would also consider instead of creating modify_ldt() just put raw syscall:
rval = tst_syscall(__NR_modify_ldt, func, &ptr, bytecount);
> +
> +#define SAFE_MODIFY_LDT(func, ptr, bytecount) \
> + safe_modify_ldt(__FILE__, __LINE__, (func), (ptr), (bytecount))
> +
> +#endif /* LAPI_LDT_H__ */
> diff --git a/testcases/cve/cve-2015-3290.c b/testcases/cve/cve-2015-3290.c
> index 63e5d92c91b830cd8066a6a6c329461b72731f32..c1c513c7c652313a6778bbdc84f0140e530650dc 100644
> --- a/testcases/cve/cve-2015-3290.c
> +++ b/testcases/cve/cve-2015-3290.c
> @@ -118,12 +118,10 @@ perhaps unsurprisingly.)
> #include "tst_test.h"
> #include "tst_timer.h"
> -#if defined(__x86_64__) || defined(__i386__)
> -
> +#if defined(__i386__) || defined(__x86_64__)
> #include <stdlib.h>
> #include <stdio.h>
> #include <inttypes.h>
> -#include <asm/ldt.h>
> #include <unistd.h>
> #include <sys/syscall.h>
We don't need <sys/syscall.h>
> #include <setjmp.h>
> @@ -132,6 +130,7 @@ perhaps unsurprisingly.)
> #include <sys/wait.h>
> #include <linux/perf_event.h>
> +#include "lapi/ldt.h"
> #include "lapi/syscalls.h"
We don't need "lapi/syscalls.h" (it's in "lapi/ldt.h").
> #include "tst_safe_pthread.h"
> @@ -187,7 +186,7 @@ static int running = 1;
> static void set_ldt(void)
> {
> /* Boring 16-bit data segment. */
> - const struct user_desc data_desc = {
> + struct user_desc data_desc = {
> .entry_number = 0,
> .base_addr = 0,
> .limit = 0xfffff,
> @@ -199,27 +198,7 @@ static void set_ldt(void)
> .useable = 0
> };
> - TEST(tst_syscall(__NR_modify_ldt, 1, &data_desc, sizeof(data_desc)));
> -
> - /*
> - * The kernel intentionally casts modify_ldt() return value
> - * to unsigned int to prevent sign extension to 64 bits. This may
> - * result in syscall() returning the value as is instead of setting
> - * errno and returning -1.
> - */
> - if (TST_RET > 0 && ((int)TST_RET) < 0) {
> - tst_res(TINFO,
> - "WARNING: Libc mishandled modify_ldt() return value");
> - TST_ERR = -(int)TST_RET;
> - TST_RET = -1;
> - }
> -
> - if (TST_RET == -1 && TST_ERR == EINVAL) {
> - tst_brk(TCONF | TTERRNO,
> - "modify_ldt: 16-bit data segments are probably disabled");
> - } else if (TST_RET != 0) {
> - tst_brk(TBROK | TTERRNO, "modify_ldt");
> - }
> + SAFE_MODIFY_LDT(1, &data_desc, sizeof(data_desc));
> }
> static void try_corrupt_stack(unsigned short *orig_ss)
> @@ -528,8 +507,6 @@ static struct tst_test test = {
> }
> };
> -#else /* defined(__x86_64__) || defined(__i386__) */
> -
> -TST_TEST_TCONF("not (i386 or x86_64)");
> -
> -#endif
> +#else
> +TST_TEST_TCONF("Test supported only on x86");
> +#endif /* defined(__i386__) || defined(__x86_64__) */
> diff --git a/testcases/cve/cve-2017-17053.c b/testcases/cve/cve-2017-17053.c
> index fe7b6d694d6ffbbce863abc1672e03ae5f419df1..7ba22fa94cee57fcb0b0b60c2462d036cb4a40c5 100644
> --- a/testcases/cve/cve-2017-17053.c
> +++ b/testcases/cve/cve-2017-17053.c
> @@ -16,8 +16,7 @@
> #include "config.h"
> #include "tst_test.h"
> -#ifdef HAVE_ASM_LDT_H
> -#include <asm/ldt.h>
> +#if defined(__i386__) || defined(__x86_64__)
> #include <pthread.h>
> #include <signal.h>
> #include <stdlib.h>
> @@ -26,6 +25,7 @@
> #include <unistd.h>
> #include <stdio.h>
> +#include "lapi/ldt.h"
> #include "lapi/syscalls.h"
nit: We don't need "lapi/syscalls.h" since it's in "lapi/ldt.h". And we don't
need <sys/syscall.h>.
> #define EXEC_USEC 5000000
> @@ -109,7 +109,7 @@ void run_test(void)
> struct user_desc desc = { .entry_number = 8191 };
> install_sighandler();
> - syscall(__NR_modify_ldt, 1, &desc, sizeof(desc));
> + SAFE_MODIFY_LDT(1, &desc, sizeof(desc));
> for (;;) {
> if (shm->do_exit)
> @@ -164,5 +164,5 @@ static struct tst_test test = {
> };
> #else
> -TST_TEST_TCONF("no asm/ldt.h header (only for i386 or x86_64)");
> -#endif
> +TST_TEST_TCONF("Test supported only on x86");
nit: for some people "x86" means i386 (32 bit intel).
> +#endif /* defined(__i386__) || defined(__x86_64__) */
> diff --git a/testcases/kernel/syscalls/fork/fork05.c b/testcases/kernel/syscalls/fork/fork05.c
> index 22edefc3686978fbb9453dffabfcbccb7ea6bb12..9aa12e16201dec8f3d2a4c99df83c4e5e25ef857 100644
> --- a/testcases/kernel/syscalls/fork/fork05.c
> +++ b/testcases/kernel/syscalls/fork/fork05.c
> @@ -55,8 +55,7 @@
> #if defined(__i386__)
> -#include "lapi/syscalls.h"
> -#include <asm/ldt.h>
> +#include "lapi/ldt.h"
> static void run(void)
> {
> @@ -76,7 +75,7 @@ static void run(void)
> ldt0.seg_not_present = 0;
> ldt0.useable = 1;
> - tst_syscall(__NR_modify_ldt, 1, &ldt0, sizeof(ldt0));
Interesting, no check for return value.
> + SAFE_MODIFY_LDT(1, &ldt0, sizeof(ldt0));
Kind regards,
Petr
> asm volatile ("movw %w0, %%fs"::"q" (7));
> asm volatile ("movl %%fs:0, %0":"=r" (lo));
More information about the ltp
mailing list