Found a (small) bug in this file: https://git.musl-libc.org/cgit/musl/tree/src/thread/i386/__set_thread_area.s If the syscall fails, the branch on line 20 is taken and %eax will be a small negative number. Then "mov $123,%al" will make syscall 0xffffff7b instead of 0x7b, since overwriting %al only overwrites the low byte of %eax. So the modify_ldt fallback has apparently never worked. Tangentially, I'm not sure why this file has so many hardcoded magic numbers and no comments to explain what they are. ~Theodore
On Sun, Aug 30, 2020 at 05:34:09PM -0700, Theodore Dubois wrote: > Found a (small) bug in this file: > https://git.musl-libc.org/cgit/musl/tree/src/thread/i386/__set_thread_area..s > > If the syscall fails, the branch on line 20 is taken and %eax will > be a small negative number. Then "mov $123,%al" will make syscall > 0xffffff7b instead of 0x7b, since overwriting %al only overwrites > the low byte of %eax. So the modify_ldt fallback has apparently > never worked. Thanks! Indeed, systems where the first syscall fails are outside the actually-supported version range (before 2.6) so it's likely that this was never actually tested. Nowadays SECCOMP_FILTER makes it easy to test, so we should actually try to test some of these things. Have you checked if it works adding xor %eax,%eax before the byte mov or changing it to a 32-bit mov? > Tangentially, I'm not sure why this file has so many hardcoded magic > numbers and no comments to explain what they are. Really it shouldn't even be external asm anymore. I'm trying to move as much external asm as possible to inline asm written in C, and this could be written entirely with __syscall() after #define SYSCALL_NO_TLS (recently added) and all the structures setup explicitly in C. I'll take a stab at doing that after fixing the bug. Rich
On Sun, Aug 30, 2020 at 09:07:10PM -0400, Rich Felker wrote:
> On Sun, Aug 30, 2020 at 05:34:09PM -0700, Theodore Dubois wrote:
> > Found a (small) bug in this file:
> > https://git.musl-libc.org/cgit/musl/tree/src/thread/i386/__set_thread_area..s
> >
> > If the syscall fails, the branch on line 20 is taken and %eax will
> > be a small negative number. Then "mov $123,%al" will make syscall
> > 0xffffff7b instead of 0x7b, since overwriting %al only overwrites
> > the low byte of %eax. So the modify_ldt fallback has apparently
> > never worked.
>
> Thanks! Indeed, systems where the first syscall fails are outside the
> actually-supported version range (before 2.6) so it's likely that this
> was never actually tested. Nowadays SECCOMP_FILTER makes it easy to
> test, so we should actually try to test some of these things. Have you
> checked if it works adding xor %eax,%eax before the byte mov or
> changing it to a 32-bit mov?
OK, I just confirmed it works after this change. I'll push a fix soon
along with a bunch of other pending commits. Thanks again for the
report.
Rich
[-- Attachment #1: Type: text/plain, Size: 1301 bytes --] On Sun, Aug 30, 2020 at 09:41:45PM -0400, Rich Felker wrote: > On Sun, Aug 30, 2020 at 09:07:10PM -0400, Rich Felker wrote: > > On Sun, Aug 30, 2020 at 05:34:09PM -0700, Theodore Dubois wrote: > > > Found a (small) bug in this file: > > > https://git.musl-libc.org/cgit/musl/tree/src/thread/i386/__set_thread_area..s > > > > > > If the syscall fails, the branch on line 20 is taken and %eax will > > > be a small negative number. Then "mov $123,%al" will make syscall > > > 0xffffff7b instead of 0x7b, since overwriting %al only overwrites > > > the low byte of %eax. So the modify_ldt fallback has apparently > > > never worked. > > > > Thanks! Indeed, systems where the first syscall fails are outside the > > actually-supported version range (before 2.6) so it's likely that this > > was never actually tested. Nowadays SECCOMP_FILTER makes it easy to > > test, so we should actually try to test some of these things. Have you > > checked if it works adding xor %eax,%eax before the byte mov or > > changing it to a 32-bit mov? > > OK, I just confirmed it works after this change. I'll push a fix soon > along with a bunch of other pending commits. Thanks again for the > report. The attached should be the equivalent in C, but it's somewhat larger. Somewhat indifferent on replacing it. Rich [-- Attachment #2: __set_thread_area.c --] [-- Type: text/plain, Size: 657 bytes --] #define SYSCALL_NO_TLS 1 #include <stdint.h> #include "syscall.h" struct user_desc { uint32_t entry_number; uint32_t base_addr; uint32_t limit; uint32_t flags; }; static int entry_number = -1; int __set_thread_area_2(void *p) { struct user_desc desc = { entry_number, (uintptr_t)p, 0xfffff, 0x51 }; int r = __syscall(SYS_set_thread_area, &desc); if (!r) { entry_number = desc.entry_number; __asm__ __volatile__ ("mov %0,%%gs" : : "r"(3+8*desc.entry_number)); return 0; } desc.entry_number = 0; r = __syscall(SYS_modify_ldt, 1, &desc, 16); if (!r) { __asm__ __volatile__ ("mov %0,%%gs" : : "r"(7)); return 1; } return r; }
On Sun, 30 Aug 2020, Rich Felker wrote:
> > Tangentially, I'm not sure why this file has so many hardcoded magic
> > numbers and no comments to explain what they are.
>
> Really it shouldn't even be external asm anymore. I'm trying to move
> as much external asm as possible to inline asm written in C, and this
> could be written entirely with __syscall() after #define
> SYSCALL_NO_TLS (recently added) and all the structures setup
> explicitly in C. I'll take a stab at doing that after fixing the bug.
I also would like an answer to a question about utter lack of comments
that your response did not directly address. This is an issue in
specialized asm implementations of math functions too, for instance.
Alexander
On Mon, Aug 31, 2020 at 09:55:37AM +0300, Alexander Monakov wrote:
> On Sun, 30 Aug 2020, Rich Felker wrote:
>
> > > Tangentially, I'm not sure why this file has so many hardcoded magic
> > > numbers and no comments to explain what they are.
> >
> > Really it shouldn't even be external asm anymore. I'm trying to move
> > as much external asm as possible to inline asm written in C, and this
> > could be written entirely with __syscall() after #define
> > SYSCALL_NO_TLS (recently added) and all the structures setup
> > explicitly in C. I'll take a stab at doing that after fixing the bug.
>
> I also would like an answer to a question about utter lack of comments
> that your response did not directly address. This is an issue in
> specialized asm implementations of math functions too, for instance.
It's just historically how it was done. In general I don't like
comments that are just translation of the programming language to
human language, which tends to be the case with asm comments, as
opposed to ones that explain purpose, contract, subtle reasons
something is done in a particular way that might not be obvious, etc.
But I do think there's room for the latter kind of comments in files
like this. For instance the saved entry number logic, that modify_ldt
is used as fallback for kernels that aren't actually supported,
how conversion of entry number to a segment register value works, etc.
The same applies to math, and I agree it's frustrating reading that
code with no comments. At the same time, a lot of the frustration is
just the density and weird flow, not the actual math logic, so I think
there's a lot more value in converting them to minimal inline asm
where the flow is in C than in adding comments. Once someone has taken
time to understand the asm enough to comment it, that's at least 90%
of the way to actually doing the conversion.
Rich
On Sun, Aug 30, 2020 at 10:51:38PM -0400, Rich Felker wrote: > The attached should be the equivalent in C, but it's somewhat larger. > Somewhat indifferent on replacing it. > > Rich I support that change. I did send some comments for that file a while ago, but actually moving this stuff to a language that is easier on the eyes than assembler is probably the better move. Plus, this way the compiler gets to optimize the access to the static variable depending on compilation mode (e.g. PIC/non-PIC, i386/i686, etc. etc.). The call/pop was always a little irksome to me. > #define SYSCALL_NO_TLS 1 > #include <stdint.h> > #include "syscall.h" > > struct user_desc { > uint32_t entry_number; > uint32_t base_addr; > uint32_t limit; > uint32_t flags; > }; > > static int entry_number = -1; > > int __set_thread_area_2(void *p) > { > struct user_desc desc = { > entry_number, (uintptr_t)p, 0xfffff, 0x51 > }; > int r = __syscall(SYS_set_thread_area, &desc); > if (!r) { > entry_number = desc.entry_number; > __asm__ __volatile__ ("mov %0,%%gs" : > : "r"(3+8*desc.entry_number)); I always wonder why people put underscores around the volatile, though. Given that volatile is always a keyword, no matter the compiler settings. Plus, in this case it is unnecessary, since the asm snippet has no outputs, so it is always volatile. > return 0; > } > desc.entry_number = 0; > r = __syscall(SYS_modify_ldt, 1, &desc, 16); > if (!r) { > __asm__ __volatile__ ("mov %0,%%gs" : > : "r"(7)); > return 1; > } > return r; > } Ciao, Markus