Problems that emerged when trying to port dosemu2

Problems that emerged when trying to port dosemu2

[bluemoon]

Hi,

I was trying to build dosemu2 (https://github.com/stsp/dosemu2) and ran
into some problems which I reported to the developer of dosemu2. He
found two issues which might be relevant to musl itself so I’d like to
report them.

The starting point was that dosemu2 crashed when trying to use DPMI to run
protected mode DOS programs. The Issue was discussed here:
https://github.com/stsp/dosemu2/issues/537
The last comments are the ones of relevance.

My knowledge of the matter is too limited to explain it in my own words, but
he summarized what’s going on here (patches are below):
https://github.com/stsp/dosemu2/issues/537#issuecomment-346177776

> The checks that you remove, are nonsense:
> they check for "ss_size" and return ENOMEM
> even for SS_DISABLE. They check for ~SS_DISABLE
> and return error for SS_AUTODISARM, even
> though it is defined in their headers. Overall
> they try to check the syscall parameters -
> something they should never do simply because
> libc does not understand the syscall parameters.
> It should just call the syscall - not more, not less.
> syscall understands its parameters, so it will
> check them correctly and return error as appropriate.
> Check from musl should be removed, and I think
> it would be good to try to submit that change.
>
> Stack-protector problem is a kernel mis-feature,
> and a very unfortunate one. We should pester
> Andy Lutomirski (@amluto) to finally fix it. :)
> I don't know if musl can accept this patch, maybe
> it can if the attribute is put under #ifdef __GNUC__
> check.

To make it work the following two patches were applied:

--- src/misc/syscall.c.orig     2017-10-31 20:13:58.000000000 +0100
+++ src/misc/syscall.c  2017-11-21 18:36:38.912082672 +0100
@@ -3,7 +3,7 @@

 #undef syscall

-long syscall(long n, ...)
+__attribute__((optimize("no-stack-protector"))) long syscall(long n, ...)
 {
        va_list ap;
        syscall_arg_t a,b,c,d,e,f;

--- src/signal/sigaltstack.c.orig       2017-10-31 20:13:58.000000000 +0100
+++ src/signal/sigaltstack.c    2017-11-21 20:56:59.740814704 +0100
@@ -4,15 +4,5 @@

 int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
 {
-       if (ss) {
-               if (ss->ss_size < MINSIGSTKSZ) {
-                       errno = ENOMEM;
-                       return -1;
-               }
-               if (ss->ss_flags & ~SS_DISABLE) {
-                       errno = EINVAL;
-                       return -1;
-               }
-       }
        return syscall(SYS_sigaltstack, ss, old);
 }

Re: Problems that emerged when trying to port dosemu2

[Szabolcs Nagy]

* bluemoon <blaumolch@mailbox.org> [2017-12-03 11:50:34 +0100]:
> My knowledge of the matter is too limited to explain it in my own words, but
> he summarized what’s going on here (patches are below):
> https://github.com/stsp/dosemu2/issues/537#issuecomment-346177776
> 
> > The checks that you remove, are nonsense:
> > they check for "ss_size" and return ENOMEM
> > even for SS_DISABLE. They check for ~SS_DISABLE
> > and return error for SS_AUTODISARM, even
> > though it is defined in their headers. Overall
> > they try to check the syscall parameters -
> > something they should never do simply because
> > libc does not understand the syscall parameters.
> > It should just call the syscall - not more, not less.
> > syscall understands its parameters, so it will
> > check them correctly and return error as appropriate.
> > Check from musl should be removed, and I think
> > it would be good to try to submit that change.
> >
> > Stack-protector problem is a kernel mis-feature,
> > and a very unfortunate one. We should pester
> > Andy Lutomirski (@amluto) to finally fix it. :)
> > I don't know if musl can accept this patch, maybe
> > it can if the attribute is put under #ifdef __GNUC__
> > check.
> 
> To make it work the following two patches were applied:
> 
> --- src/misc/syscall.c.orig     2017-10-31 20:13:58.000000000 +0100
> +++ src/misc/syscall.c  2017-11-21 18:36:38.912082672 +0100
> @@ -3,7 +3,7 @@
> 
>  #undef syscall
> 
> -long syscall(long n, ...)
> +__attribute__((optimize("no-stack-protector"))) long syscall(long n, ...)
>  {

changing fs/gs behind the back of the c runtime is not
guaranteed to work, but it makes sense to me to compile
syscall.c without ssp instrumentation to allow certain hacks.
(but i think this should be done in the makefile)

>         va_list ap;
>         syscall_arg_t a,b,c,d,e,f;
> 
> --- src/signal/sigaltstack.c.orig       2017-10-31 20:13:58.000000000 +0100
> +++ src/signal/sigaltstack.c    2017-11-21 20:56:59.740814704 +0100
> @@ -4,15 +4,5 @@
> 
>  int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
>  {
> -       if (ss) {
> -               if (ss->ss_size < MINSIGSTKSZ) {
> -                       errno = ENOMEM;
> -                       return -1;
> -               }

i think this part has to be kept for conformance reasons:
the kernel does not check MINSIGSTKSZ (it does not even
know how it is defined in musl, so it is musl abi, not
kernel abi), but posix requires the check.

> -               if (ss->ss_flags & ~SS_DISABLE) {
> -                       errno = EINVAL;
> -                       return -1;
> -               }

this is another conformance check, but one can argue
that linux extensions should be allowed here.
(it's unfortunate that some useful linux extensions
are in conflict with posix requirements..)

> -       }
>         return syscall(SYS_sigaltstack, ss, old);
>  }

Re: Problems that emerged when trying to port dosemu2

[Rich Felker]

On Sun, Dec 03, 2017 at 03:49:20PM +0100, Szabolcs Nagy wrote:
> * bluemoon <blaumolch@mailbox.org> [2017-12-03 11:50:34 +0100]:
> > My knowledge of the matter is too limited to explain it in my own words, but
> > he summarized what’s going on here (patches are below):
> > https://github.com/stsp/dosemu2/issues/537#issuecomment-346177776
> > 
> > > The checks that you remove, are nonsense:
> > > they check for "ss_size" and return ENOMEM
> > > even for SS_DISABLE. They check for ~SS_DISABLE
> > > and return error for SS_AUTODISARM, even
> > > though it is defined in their headers. Overall
> > > they try to check the syscall parameters -
> > > something they should never do simply because
> > > libc does not understand the syscall parameters.
> > > It should just call the syscall - not more, not less.
> > > syscall understands its parameters, so it will
> > > check them correctly and return error as appropriate.
> > > Check from musl should be removed, and I think
> > > it would be good to try to submit that change.
> > >
> > > Stack-protector problem is a kernel mis-feature,
> > > and a very unfortunate one. We should pester
> > > Andy Lutomirski (@amluto) to finally fix it. :)
> > > I don't know if musl can accept this patch, maybe
> > > it can if the attribute is put under #ifdef __GNUC__
> > > check.
> > 
> > To make it work the following two patches were applied:
> > 
> > --- src/misc/syscall.c.orig     2017-10-31 20:13:58.000000000 +0100
> > +++ src/misc/syscall.c  2017-11-21 18:36:38.912082672 +0100
> > @@ -3,7 +3,7 @@
> > 
> >  #undef syscall
> > 
> > -long syscall(long n, ...)
> > +__attribute__((optimize("no-stack-protector"))) long syscall(long n, ...)
> >  {
> 
> changing fs/gs behind the back of the c runtime is not
> guaranteed to work, but it makes sense to me to compile
> syscall.c without ssp instrumentation to allow certain hacks.
> (but i think this should be done in the makefile)

I'm not clear why this would even work on glibc, since %gs is used to
access the vdso syscall pointer. I don't think it makes sense to treat
syscall() specially here. If the thread pointer register is not valid,
then the ABI is not being satisfied and that's all there is to say.
Programs that have changed the thread pointer in a thread must refrain
from doing anything that could cause a call into libc. If they need to
make syscalls, they can do it via [inline] asm; they're already using
target-specific asm anyway if they're changing %fs/%gs.

> >         va_list ap;
> >         syscall_arg_t a,b,c,d,e,f;
> > 
> > --- src/signal/sigaltstack.c.orig       2017-10-31 20:13:58.000000000 +0100
> > +++ src/signal/sigaltstack.c    2017-11-21 20:56:59.740814704 +0100
> > @@ -4,15 +4,5 @@
> > 
> >  int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
> >  {
> > -       if (ss) {
> > -               if (ss->ss_size < MINSIGSTKSZ) {
> > -                       errno = ENOMEM;
> > -                       return -1;
> > -               }
> 
> i think this part has to be kept for conformance reasons:
> the kernel does not check MINSIGSTKSZ (it does not even
> know how it is defined in musl, so it is musl abi, not
> kernel abi), but posix requires the check.

Indeed, but POSIX says:

    "If it is set to SS_DISABLE, the stack is disabled and ss_sp and
    ss_size are ignored."

so it's a bug to be checking ss_size when ss_flags == SS_DISABLE. We
should only check ss_size if !(ss_flags & SS_DISABLE) or similar.

> > -               if (ss->ss_flags & ~SS_DISABLE) {
> > -                       errno = EINVAL;
> > -                       return -1;
> > -               }
> 
> this is another conformance check, but one can argue
> that linux extensions should be allowed here.
> (it's unfortunate that some useful linux extensions
> are in conflict with posix requirements..)

Yes, this one is a requirement and I don't see any way around it...

Rich

Re: Problems that emerged when trying to port dosemu2

[bluemoon]

Am 03.12.2017 um 23:01 schrieb Rich Felker:
> On Sun, Dec 03, 2017 at 03:49:20PM +0100, Szabolcs Nagy wrote:
>> * bluemoon <blaumolch@mailbox.org> [2017-12-03 11:50:34 +0100]:
>>> My knowledge of the matter is too limited to explain it in my own words, but
>>> he summarized what’s going on here (patches are below):
>>> https://github.com/stsp/dosemu2/issues/537#issuecomment-346177776
>>>
>>>> The checks that you remove, are nonsense:
>>>> they check for "ss_size" and return ENOMEM
>>>> even for SS_DISABLE. They check for ~SS_DISABLE
>>>> and return error for SS_AUTODISARM, even
>>>> though it is defined in their headers. Overall
>>>> they try to check the syscall parameters -
>>>> something they should never do simply because
>>>> libc does not understand the syscall parameters.
>>>> It should just call the syscall - not more, not less.
>>>> syscall understands its parameters, so it will
>>>> check them correctly and return error as appropriate.
>>>> Check from musl should be removed, and I think
>>>> it would be good to try to submit that change.
>>>>
>>>> Stack-protector problem is a kernel mis-feature,
>>>> and a very unfortunate one. We should pester
>>>> Andy Lutomirski (@amluto) to finally fix it. :)
>>>> I don't know if musl can accept this patch, maybe
>>>> it can if the attribute is put under #ifdef __GNUC__
>>>> check.
>>>
>>> To make it work the following two patches were applied:
>>>
>>> --- src/misc/syscall.c.orig     2017-10-31 20:13:58.000000000 +0100
>>> +++ src/misc/syscall.c  2017-11-21 18:36:38.912082672 +0100
>>> @@ -3,7 +3,7 @@
>>>
>>>  #undef syscall
>>>
>>> -long syscall(long n, ...)
>>> +__attribute__((optimize("no-stack-protector"))) long syscall(long n, ...)
>>>  {
>>
>> changing fs/gs behind the back of the c runtime is not
>> guaranteed to work, but it makes sense to me to compile
>> syscall.c without ssp instrumentation to allow certain hacks.
>> (but i think this should be done in the makefile)
> 
> I'm not clear why this would even work on glibc, since %gs is used to
> access the vdso syscall pointer. I don't think it makes sense to treat
> syscall() specially here. If the thread pointer register is not valid,
> then the ABI is not being satisfied and that's all there is to say.
> Programs that have changed the thread pointer in a thread must refrain
> from doing anything that could cause a call into libc. If they need to
> make syscalls, they can do it via [inline] asm; they're already using
> target-specific asm anyway if they're changing %fs/%gs.
> 
>>>         va_list ap;
>>>         syscall_arg_t a,b,c,d,e,f;
>>>
>>> --- src/signal/sigaltstack.c.orig       2017-10-31 20:13:58.000000000 +0100
>>> +++ src/signal/sigaltstack.c    2017-11-21 20:56:59.740814704 +0100
>>> @@ -4,15 +4,5 @@
>>>
>>>  int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
>>>  {
>>> -       if (ss) {
>>> -               if (ss->ss_size < MINSIGSTKSZ) {
>>> -                       errno = ENOMEM;
>>> -                       return -1;
>>> -               }
>>
>> i think this part has to be kept for conformance reasons:
>> the kernel does not check MINSIGSTKSZ (it does not even
>> know how it is defined in musl, so it is musl abi, not
>> kernel abi), but posix requires the check.
> 
> Indeed, but POSIX says:
> 
>     "If it is set to SS_DISABLE, the stack is disabled and ss_sp and
>     ss_size are ignored."
> 
> so it's a bug to be checking ss_size when ss_flags == SS_DISABLE. We
> should only check ss_size if !(ss_flags & SS_DISABLE) or similar.
> 
>>> -               if (ss->ss_flags & ~SS_DISABLE) {
>>> -                       errno = EINVAL;
>>> -                       return -1;
>>> -               }
>>
>> this is another conformance check, but one can argue
>> that linux extensions should be allowed here.
>> (it's unfortunate that some useful linux extensions
>> are in conflict with posix requirements..)
> 
> Yes, this one is a requirement and I don't see any way around it...

Thank you for your replies!

The developer of dosemu2 was able to implement some work-arounds so that
now it’s possible to run protected mode programs.

However, a remaining question is that if linux extensions aren’t allowed
why is

#define SS_AUTODISARM (1U << 31)
#define SS_FLAG_BITS SS_AUTODISARM

defined in signal.h? This confuses the detections.