mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] i386 __set_thread_area will crash if the syscall fails
@ 2020-08-31  0:34 Theodore Dubois
  2020-08-31  1:07 ` Rich Felker
  0 siblings, 1 reply; 7+ messages in thread
From: Theodore Dubois @ 2020-08-31  0:34 UTC (permalink / raw)
  To: musl

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] i386 __set_thread_area will crash if the syscall fails
  2020-08-31  0:34 [musl] i386 __set_thread_area will crash if the syscall fails Theodore Dubois
@ 2020-08-31  1:07 ` Rich Felker
  2020-08-31  1:41   ` Rich Felker
  2020-08-31  6:55   ` Alexander Monakov
  0 siblings, 2 replies; 7+ messages in thread
From: Rich Felker @ 2020-08-31  1:07 UTC (permalink / raw)
  To: musl; +Cc: Theodore Dubois

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] i386 __set_thread_area will crash if the syscall fails
  2020-08-31  1:07 ` Rich Felker
@ 2020-08-31  1:41   ` Rich Felker
  2020-08-31  2:51     ` Rich Felker
  2020-08-31  6:55   ` Alexander Monakov
  1 sibling, 1 reply; 7+ messages in thread
From: Rich Felker @ 2020-08-31  1:41 UTC (permalink / raw)
  To: musl; +Cc: Theodore Dubois

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] i386 __set_thread_area will crash if the syscall fails
  2020-08-31  1:41   ` Rich Felker
@ 2020-08-31  2:51     ` Rich Felker
  2020-09-01 18:20       ` Markus Wichmann
  0 siblings, 1 reply; 7+ messages in thread
From: Rich Felker @ 2020-08-31  2:51 UTC (permalink / raw)
  To: musl; +Cc: Theodore Dubois

[-- 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;
}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] i386 __set_thread_area will crash if the syscall fails
  2020-08-31  1:07 ` Rich Felker
  2020-08-31  1:41   ` Rich Felker
@ 2020-08-31  6:55   ` Alexander Monakov
  2020-08-31 12:36     ` Rich Felker
  1 sibling, 1 reply; 7+ messages in thread
From: Alexander Monakov @ 2020-08-31  6:55 UTC (permalink / raw)
  To: musl; +Cc: Theodore Dubois

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] i386 __set_thread_area will crash if the syscall fails
  2020-08-31  6:55   ` Alexander Monakov
@ 2020-08-31 12:36     ` Rich Felker
  0 siblings, 0 replies; 7+ messages in thread
From: Rich Felker @ 2020-08-31 12:36 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: musl, Theodore Dubois

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] i386 __set_thread_area will crash if the syscall fails
  2020-08-31  2:51     ` Rich Felker
@ 2020-09-01 18:20       ` Markus Wichmann
  0 siblings, 0 replies; 7+ messages in thread
From: Markus Wichmann @ 2020-09-01 18:20 UTC (permalink / raw)
  To: musl

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-09-01 18:20 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-31  0:34 [musl] i386 __set_thread_area will crash if the syscall fails Theodore Dubois
2020-08-31  1:07 ` Rich Felker
2020-08-31  1:41   ` Rich Felker
2020-08-31  2:51     ` Rich Felker
2020-09-01 18:20       ` Markus Wichmann
2020-08-31  6:55   ` Alexander Monakov
2020-08-31 12:36     ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).