[LTP] [PATCH v5 1/3] syscalls/modify_ldt: Add lapi/ldt.h
Ricardo B. Marlière
rbm@suse.com
Tue Apr 1 15:55:20 CEST 2025
Hi Petr,
On Tue Apr 1, 2025 at 5:48 AM -03, Petr Vorel wrote:
> 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).
>
Makes sense, I'll amend the commit log.
> 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.
I will take a look and see about that in a future series, if I find
anything useful.
>
> 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:
Actually we need this in modify_ldt01.c:
TST_EXP_FAIL(modify_ldt(tc->tfunc, tc->ptr, tc->bytecount), tc->exp_errno);
and
TST_EXP_POSITIVE(modify_ldt(tc->tfunc, tc->ptr, tc->bytecount));
>
> 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").
Ack.
>
>> #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>.
Ack, thanks!
>
>> #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).
Indeed. I borrowed this from the kvm suite, I think we can keep it.
>
>> +#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));
Thanks for reviewing,
- Ricardo.
More information about the ltp
mailing list