| Message ID | 20250512-fixes-modify_ldt-v2-1-eaef5577e44e@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] ldt.h: Add workaround for x86_64 | expand |
| Context | Check | Description |
|---|---|---|
| ltpci/debian_stable_powerpc64le-linux-gnu-gcc_ppc64el | success | success |
| ltpci/debian_stable_s390x-linux-gnu-gcc_s390x | success | success |
| ltpci/debian_stable_aarch64-linux-gnu-gcc_arm64 | success | success |
| ltpci/debian_stable_gcc | success | success |
| ltpci/debian_stable_gcc | success | success |
| ltpci/ubuntu_jammy_gcc | success | success |
| ltpci/quay-io-centos-centos_stream9_gcc | success | success |
| ltpci/fedora_latest_clang | success | success |
| ltpci/alpine_latest_gcc | success | success |
| ltpci/debian_testing_clang | success | success |
| ltpci/debian_testing_gcc | success | success |
| ltpci/debian_oldstable_clang | success | success |
| ltpci/opensuse-archive_42-2_gcc | success | success |
| ltpci/opensuse-leap_latest_gcc | success | success |
| ltpci/ubuntu_bionic_gcc | success | success |
| ltpci/debian_oldstable_gcc | success | success |
Hi, Reviewed-by: Martin Doucha <mdoucha@suse.cz> On 12. 05. 25 12:05, Ricardo B. Marlière wrote: > From: Ricardo B. Marlière <rbm@suse.com> > > The commit be0aaca2f742 ("syscalls/modify_ldt: Add lapi/ldt.h") left behind > an important factor of modify_ldt(): the kernel intentionally casts the > return value to unsigned int. This was handled in > testcases/cve/cve-2015-3290.c but was removed. Add it back to the relevant > file. > > Reported-by: Martin Doucha <mdoucha@suse.cz> > Signed-off-by: Ricardo B. Marlière <rbm@suse.com> > --- > Changes in v2: > - Added TBROK for any ret != 0 in modify_ldt call in cve-2015-3290.c > - Link to v1: https://lore.kernel.org/r/20250507-fixes-modify_ldt-v1-1-70a2694cfddc@suse.com > --- > include/lapi/ldt.h | 22 +++++++++++++++++++++- > testcases/cve/cve-2015-3290.c | 8 +++++++- > 2 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/include/lapi/ldt.h b/include/lapi/ldt.h > index 6b5a2d59cb41bfc24eb5ac26c3d47d49fb8ff78f..173321dd9ac34ba87eff0eee960635f30d878991 100644 > --- a/include/lapi/ldt.h > +++ b/include/lapi/ldt.h > @@ -31,7 +31,27 @@ struct user_desc { > static inline int modify_ldt(int func, const struct user_desc *ptr, > unsigned long bytecount) > { > - return tst_syscall(__NR_modify_ldt, func, ptr, bytecount); > + long rval; > + > + errno = 0; > + rval = tst_syscall(__NR_modify_ldt, func, ptr, bytecount); > + > +#ifdef __x86_64__ > + /* > + * 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 (rval > 0 && (int)rval < 0) { > + tst_res(TINFO, > + "WARNING: Libc mishandled modify_ldt() return value"); > + errno = -(int)errno; > + rval = -1; > + } > +#endif /* __x86_64__ */ > + > + return rval; > } > > static inline int safe_modify_ldt(const char *file, const int lineno, int func, > diff --git a/testcases/cve/cve-2015-3290.c b/testcases/cve/cve-2015-3290.c > index 8ec1d53bbb5a9f3e7761d39855d34f593e118a28..e70742acc87c39088953e02f16146b7b58a75fd1 100644 > --- a/testcases/cve/cve-2015-3290.c > +++ b/testcases/cve/cve-2015-3290.c > @@ -197,7 +197,13 @@ static void set_ldt(void) > .useable = 0 > }; > > - SAFE_MODIFY_LDT(1, &data_desc, sizeof(data_desc)); > + TEST(modify_ldt(1, &data_desc, sizeof(data_desc))); > + 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"); > + } > } > > static void try_corrupt_stack(unsigned short *orig_ss) > > --- > base-commit: b070a5692e035ec12c3d3c7a7e9e97c270fd4d7d > change-id: 20250507-fixes-modify_ldt-ebcfdd2a7d30 > > Best regards,
On Mon May 12, 2025 at 7:10 AM -03, Martin Doucha wrote: > Hi, > Reviewed-by: Martin Doucha <mdoucha@suse.cz> Thanks for reviewing and all the help! - Ricardo. > > On 12. 05. 25 12:05, Ricardo B. Marlière wrote: >> From: Ricardo B. Marlière <rbm@suse.com> >> >> The commit be0aaca2f742 ("syscalls/modify_ldt: Add lapi/ldt.h") left behind >> an important factor of modify_ldt(): the kernel intentionally casts the >> return value to unsigned int. This was handled in >> testcases/cve/cve-2015-3290.c but was removed. Add it back to the relevant >> file. >> >> Reported-by: Martin Doucha <mdoucha@suse.cz> >> Signed-off-by: Ricardo B. Marlière <rbm@suse.com> >> --- >> Changes in v2: >> - Added TBROK for any ret != 0 in modify_ldt call in cve-2015-3290.c >> - Link to v1: https://lore.kernel.org/r/20250507-fixes-modify_ldt-v1-1-70a2694cfddc@suse.com >> --- >> include/lapi/ldt.h | 22 +++++++++++++++++++++- >> testcases/cve/cve-2015-3290.c | 8 +++++++- >> 2 files changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/include/lapi/ldt.h b/include/lapi/ldt.h >> index 6b5a2d59cb41bfc24eb5ac26c3d47d49fb8ff78f..173321dd9ac34ba87eff0eee960635f30d878991 100644 >> --- a/include/lapi/ldt.h >> +++ b/include/lapi/ldt.h >> @@ -31,7 +31,27 @@ struct user_desc { >> static inline int modify_ldt(int func, const struct user_desc *ptr, >> unsigned long bytecount) >> { >> - return tst_syscall(__NR_modify_ldt, func, ptr, bytecount); >> + long rval; >> + >> + errno = 0; >> + rval = tst_syscall(__NR_modify_ldt, func, ptr, bytecount); >> + >> +#ifdef __x86_64__ >> + /* >> + * 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 (rval > 0 && (int)rval < 0) { >> + tst_res(TINFO, >> + "WARNING: Libc mishandled modify_ldt() return value"); >> + errno = -(int)errno; >> + rval = -1; >> + } >> +#endif /* __x86_64__ */ >> + >> + return rval; >> } >> >> static inline int safe_modify_ldt(const char *file, const int lineno, int func, >> diff --git a/testcases/cve/cve-2015-3290.c b/testcases/cve/cve-2015-3290.c >> index 8ec1d53bbb5a9f3e7761d39855d34f593e118a28..e70742acc87c39088953e02f16146b7b58a75fd1 100644 >> --- a/testcases/cve/cve-2015-3290.c >> +++ b/testcases/cve/cve-2015-3290.c >> @@ -197,7 +197,13 @@ static void set_ldt(void) >> .useable = 0 >> }; >> >> - SAFE_MODIFY_LDT(1, &data_desc, sizeof(data_desc)); >> + TEST(modify_ldt(1, &data_desc, sizeof(data_desc))); >> + 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"); >> + } >> } >> >> static void try_corrupt_stack(unsigned short *orig_ss) >> >> --- >> base-commit: b070a5692e035ec12c3d3c7a7e9e97c270fd4d7d >> change-id: 20250507-fixes-modify_ldt-ebcfdd2a7d30 >> >> Best regards,
Hi! On 5/12/25 12:05, Ricardo B. Marlière via ltp wrote: > From: Ricardo B. Marlière <rbm@suse.com> > > The commit be0aaca2f742 ("syscalls/modify_ldt: Add lapi/ldt.h") left behind > an important factor of modify_ldt(): the kernel intentionally casts the > return value to unsigned int. This was handled in > testcases/cve/cve-2015-3290.c but was removed. Add it back to the relevant > file. > > Reported-by: Martin Doucha <mdoucha@suse.cz> > Signed-off-by: Ricardo B. Marlière <rbm@suse.com> > --- > Changes in v2: > - Added TBROK for any ret != 0 in modify_ldt call in cve-2015-3290.c > - Link to v1: https://lore.kernel.org/r/20250507-fixes-modify_ldt-v1-1-70a2694cfddc@suse.com > --- > include/lapi/ldt.h | 22 +++++++++++++++++++++- > testcases/cve/cve-2015-3290.c | 8 +++++++- > 2 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/include/lapi/ldt.h b/include/lapi/ldt.h > index 6b5a2d59cb41bfc24eb5ac26c3d47d49fb8ff78f..173321dd9ac34ba87eff0eee960635f30d878991 100644 > --- a/include/lapi/ldt.h > +++ b/include/lapi/ldt.h > @@ -31,7 +31,27 @@ struct user_desc { > static inline int modify_ldt(int func, const struct user_desc *ptr, > unsigned long bytecount) > { > - return tst_syscall(__NR_modify_ldt, func, ptr, bytecount); > + long rval; > + > + errno = 0; > + rval = tst_syscall(__NR_modify_ldt, func, ptr, bytecount); > + > +#ifdef __x86_64__ > + /* > + * 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 (rval > 0 && (int)rval < 0) { > + tst_res(TINFO, > + "WARNING: Libc mishandled modify_ldt() return value"); > + errno = -(int)errno; > + rval = -1; > + } > +#endif /* __x86_64__ */ This code is correct, but check comment below.. > + > + return rval; > } > > static inline int safe_modify_ldt(const char *file, const int lineno, int func, > diff --git a/testcases/cve/cve-2015-3290.c b/testcases/cve/cve-2015-3290.c > index 8ec1d53bbb5a9f3e7761d39855d34f593e118a28..e70742acc87c39088953e02f16146b7b58a75fd1 100644 > --- a/testcases/cve/cve-2015-3290.c > +++ b/testcases/cve/cve-2015-3290.c > @@ -197,7 +197,13 @@ static void set_ldt(void) > .useable = 0 > }; > > - SAFE_MODIFY_LDT(1, &data_desc, sizeof(data_desc)); > + TEST(modify_ldt(1, &data_desc, sizeof(data_desc))); > + 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"); > + } What about handling this in safe_modify_ldt, so all tests using SAFE_MODIFY_LDT() will use it? > } > > static void try_corrupt_stack(unsigned short *orig_ss) > > --- > base-commit: b070a5692e035ec12c3d3c7a7e9e97c270fd4d7d > change-id: 20250507-fixes-modify_ldt-ebcfdd2a7d30 > > Best regards, - Andrea
Hi, it turns out that there's a mistake in this patch. See below. On 12. 05. 25 12:05, Ricardo B. Marlière wrote: > From: Ricardo B. Marlière <rbm@suse.com> > > The commit be0aaca2f742 ("syscalls/modify_ldt: Add lapi/ldt.h") left behind > an important factor of modify_ldt(): the kernel intentionally casts the > return value to unsigned int. This was handled in > testcases/cve/cve-2015-3290.c but was removed. Add it back to the relevant > file. > > Reported-by: Martin Doucha <mdoucha@suse.cz> > Signed-off-by: Ricardo B. Marlière <rbm@suse.com> > --- > Changes in v2: > - Added TBROK for any ret != 0 in modify_ldt call in cve-2015-3290.c > - Link to v1: https://lore.kernel.org/r/20250507-fixes-modify_ldt-v1-1-70a2694cfddc@suse.com > --- > include/lapi/ldt.h | 22 +++++++++++++++++++++- > testcases/cve/cve-2015-3290.c | 8 +++++++- > 2 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/include/lapi/ldt.h b/include/lapi/ldt.h > index 6b5a2d59cb41bfc24eb5ac26c3d47d49fb8ff78f..173321dd9ac34ba87eff0eee960635f30d878991 100644 > --- a/include/lapi/ldt.h > +++ b/include/lapi/ldt.h > @@ -31,7 +31,27 @@ struct user_desc { > static inline int modify_ldt(int func, const struct user_desc *ptr, > unsigned long bytecount) > { > - return tst_syscall(__NR_modify_ldt, func, ptr, bytecount); > + long rval; > + > + errno = 0; > + rval = tst_syscall(__NR_modify_ldt, func, ptr, bytecount); > + > +#ifdef __x86_64__ > + /* > + * 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 (rval > 0 && (int)rval < 0) { > + tst_res(TINFO, > + "WARNING: Libc mishandled modify_ldt() return value"); > + errno = -(int)errno; I've completely missed that this line is supposed to be: errno = -(int)rval; > + rval = -1; > + } > +#endif /* __x86_64__ */ > + > + return rval; > } > > static inline int safe_modify_ldt(const char *file, const int lineno, int func, > diff --git a/testcases/cve/cve-2015-3290.c b/testcases/cve/cve-2015-3290.c > index 8ec1d53bbb5a9f3e7761d39855d34f593e118a28..e70742acc87c39088953e02f16146b7b58a75fd1 100644 > --- a/testcases/cve/cve-2015-3290.c > +++ b/testcases/cve/cve-2015-3290.c > @@ -197,7 +197,13 @@ static void set_ldt(void) > .useable = 0 > }; > > - SAFE_MODIFY_LDT(1, &data_desc, sizeof(data_desc)); > + TEST(modify_ldt(1, &data_desc, sizeof(data_desc))); > + 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"); > + } > } > > static void try_corrupt_stack(unsigned short *orig_ss) > > --- > base-commit: b070a5692e035ec12c3d3c7a7e9e97c270fd4d7d > change-id: 20250507-fixes-modify_ldt-ebcfdd2a7d30 > > Best regards,
On Wed May 21, 2025 at 8:31 AM -03, Martin Doucha wrote: > Hi, > it turns out that there's a mistake in this patch. See below. > > On 12. 05. 25 12:05, Ricardo B. Marlière wrote: >> From: Ricardo B. Marlière <rbm@suse.com> >> >> The commit be0aaca2f742 ("syscalls/modify_ldt: Add lapi/ldt.h") left behind >> an important factor of modify_ldt(): the kernel intentionally casts the >> return value to unsigned int. This was handled in >> testcases/cve/cve-2015-3290.c but was removed. Add it back to the relevant >> file. >> >> Reported-by: Martin Doucha <mdoucha@suse.cz> >> Signed-off-by: Ricardo B. Marlière <rbm@suse.com> >> --- >> Changes in v2: >> - Added TBROK for any ret != 0 in modify_ldt call in cve-2015-3290.c >> - Link to v1: https://lore.kernel.org/r/20250507-fixes-modify_ldt-v1-1-70a2694cfddc@suse.com >> --- >> include/lapi/ldt.h | 22 +++++++++++++++++++++- >> testcases/cve/cve-2015-3290.c | 8 +++++++- >> 2 files changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/include/lapi/ldt.h b/include/lapi/ldt.h >> index 6b5a2d59cb41bfc24eb5ac26c3d47d49fb8ff78f..173321dd9ac34ba87eff0eee960635f30d878991 100644 >> --- a/include/lapi/ldt.h >> +++ b/include/lapi/ldt.h >> @@ -31,7 +31,27 @@ struct user_desc { >> static inline int modify_ldt(int func, const struct user_desc *ptr, >> unsigned long bytecount) >> { >> - return tst_syscall(__NR_modify_ldt, func, ptr, bytecount); >> + long rval; >> + >> + errno = 0; >> + rval = tst_syscall(__NR_modify_ldt, func, ptr, bytecount); >> + >> +#ifdef __x86_64__ >> + /* >> + * 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 (rval > 0 && (int)rval < 0) { >> + tst_res(TINFO, >> + "WARNING: Libc mishandled modify_ldt() return value"); >> + errno = -(int)errno; > > I've completely missed that this line is supposed to be: > errno = -(int)rval; oops. I'm very sorry about that. Will send a fix right now. > >> + rval = -1; >> + } >> +#endif /* __x86_64__ */ >> + >> + return rval; >> } >> >> static inline int safe_modify_ldt(const char *file, const int lineno, int func, >> diff --git a/testcases/cve/cve-2015-3290.c b/testcases/cve/cve-2015-3290.c >> index 8ec1d53bbb5a9f3e7761d39855d34f593e118a28..e70742acc87c39088953e02f16146b7b58a75fd1 100644 >> --- a/testcases/cve/cve-2015-3290.c >> +++ b/testcases/cve/cve-2015-3290.c >> @@ -197,7 +197,13 @@ static void set_ldt(void) >> .useable = 0 >> }; >> >> - SAFE_MODIFY_LDT(1, &data_desc, sizeof(data_desc)); >> + TEST(modify_ldt(1, &data_desc, sizeof(data_desc))); >> + 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"); >> + } >> } >> >> static void try_corrupt_stack(unsigned short *orig_ss) >> >> --- >> base-commit: b070a5692e035ec12c3d3c7a7e9e97c270fd4d7d >> change-id: 20250507-fixes-modify_ldt-ebcfdd2a7d30 >> >> Best regards,
diff --git a/include/lapi/ldt.h b/include/lapi/ldt.h index 6b5a2d59cb41bfc24eb5ac26c3d47d49fb8ff78f..173321dd9ac34ba87eff0eee960635f30d878991 100644 --- a/include/lapi/ldt.h +++ b/include/lapi/ldt.h @@ -31,7 +31,27 @@ struct user_desc { static inline int modify_ldt(int func, const struct user_desc *ptr, unsigned long bytecount) { - return tst_syscall(__NR_modify_ldt, func, ptr, bytecount); + long rval; + + errno = 0; + rval = tst_syscall(__NR_modify_ldt, func, ptr, bytecount); + +#ifdef __x86_64__ + /* + * 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 (rval > 0 && (int)rval < 0) { + tst_res(TINFO, + "WARNING: Libc mishandled modify_ldt() return value"); + errno = -(int)errno; + rval = -1; + } +#endif /* __x86_64__ */ + + return rval; } static inline int safe_modify_ldt(const char *file, const int lineno, int func, diff --git a/testcases/cve/cve-2015-3290.c b/testcases/cve/cve-2015-3290.c index 8ec1d53bbb5a9f3e7761d39855d34f593e118a28..e70742acc87c39088953e02f16146b7b58a75fd1 100644 --- a/testcases/cve/cve-2015-3290.c +++ b/testcases/cve/cve-2015-3290.c @@ -197,7 +197,13 @@ static void set_ldt(void) .useable = 0 }; - SAFE_MODIFY_LDT(1, &data_desc, sizeof(data_desc)); + TEST(modify_ldt(1, &data_desc, sizeof(data_desc))); + 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"); + } } static void try_corrupt_stack(unsigned short *orig_ss)