Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 1 May 2018 14:49:00 -0700
From: Andre McCurdy <armccurdy@...il.com>
To: musl@...ts.openwall.com
Cc: Patrick Oppenlander <patrick.oppenlander@...il.com>
Subject: Re: Some questions

On Tue, May 1, 2018 at 10:35 AM, Rich Felker <dalias@...c.org> wrote:
> On Tue, May 01, 2018 at 11:52:33AM -0400, Rich Felker wrote:
>> On Tue, May 01, 2018 at 12:34:13PM +1000, Patrick Oppenlander wrote:
>> > On Tue, May 1, 2018 at 1:31 AM, Rich Felker <dalias@...c.org> wrote:
>> > > On Mon, Apr 30, 2018 at 03:29:39PM +1000, Patrick Oppenlander wrote:
>> > >> Actually, my biggest issue with getcwd is that it allocates a PATH_MAX
>> > >> sized buffer on the stack. That's painful on deeply embedded stuff.
>> > >
>> > > That's unrelated, and could/should be fixed by the attached patch I
>> > > think.
>> >
>> > Unfortunately that fails to build on arm with:
>> >
>> > src/unistd/getcwd.c: In function 'getcwd':
>> > src/unistd/getcwd.c:25:1: error: r7 cannot be used in asm here
>>
>> Then that's a bug we need to fix or work around elsewhere.
>> Non-arch-specific source files can't be constrained not to use
>> perfectly valid C constructs because gcc breaks on them for particular
>> archs.
>>
>> I believe it's related to the thumb+framepointer issue that was raised
>> a while back, but I forget how that ended and if we ever solved it.
>>
>> > I was also having a go at resolving the stack & the buffer size issue
>> > and came up with the attached (untested) patch.
>> >
>> > Patrick
>>
>> > diff --git a/src/unistd/getcwd.c b/src/unistd/getcwd.c
>> > index 103fbbb5..306dbc4f 100644
>> > --- a/src/unistd/getcwd.c
>> > +++ b/src/unistd/getcwd.c
>> > @@ -3,17 +3,10 @@
>> >  #include <limits.h>
>> >  #include <string.h>
>> >  #include "syscall.h"
>> > +#include "libc.h"
>> >
>> > -char *getcwd(char *buf, size_t size)
>> > +static char *do_getcwd(char *buf, size_t size)
>> >  {
>> > -   char tmp[PATH_MAX];
>> > -   if (!buf) {
>> > -           buf = tmp;
>> > -           size = PATH_MAX;
>> > -   } else if (!size) {
>> > -           errno = EINVAL;
>> > -           return 0;
>> > -   }
>> >     long ret = syscall(SYS_getcwd, buf, size);
>> >     if (ret < 0)
>> >             return 0;
>> > @@ -21,5 +14,37 @@ char *getcwd(char *buf, size_t size)
>> >             errno = ENOENT;
>> >             return 0;
>> >     }
>> > -   return buf == tmp ? strdup(buf) : buf;
>> > +   return buf;
>> > +}
>> > +
>> > +static char *getcwd_glibc(size_t size)
>> > +{
>> > +   char tmp[PATH_MAX];
>> > +   if (!do_getcwd(tmp, sizeof tmp))
>> > +           return 0;
>> > +   size_t len = strlen(tmp) + 1;
>> > +   if (!size)
>> > +           size = len;
>> > +   else if (size < len) {
>> > +           errno = ERANGE;
>> > +           return 0;
>> > +   }
>> > +   char *buf = malloc(size);
>> > +   if (!buf) {
>> > +           errno = ENOMEM;
>> > +           return 0;
>> > +   }
>> > +   memcpy(buf, tmp, len);
>> > +   return buf;
>> > +}
>> > +
>> > +char *getcwd(char *buf, size_t size)
>> > +{
>> > +   if (!buf)
>> > +           return getcwd_glibc(size);
>> > +   if (!size) {
>> > +           errno = EINVAL;
>> > +           return 0;
>> > +   }
>> > +   return do_getcwd(buf, size);
>> >  }
>>
>> This isn't acceptable. It makes the code much larger (at the source
>> level) and harder to read, and the only reason it works is failure of
>> gcc to optimize heavily. It could just as easily still end up using
>> the full PATH_MAX space on the stack, if gcc inlines and hoists stuff,
>> or if gcc wanted to be really awful it could still end up using a
>> frame pointer.
>>
>> Let's look back at the framepointer mess and see if there's a way to
>> get gcc not to break. If not we may need to skip inline syscalls and
>> call out to the extern __syscall when building for thumb, but I'd
>> really rather not have to do that.
>
> Looking back, it seems where we left it is just that you need to make
> sure frame pointer is disabled if building as thumb. But that's not
> reliable because gcc forcibly re-enables frame pointer (including
> frame pointer ABI constraints, which it doesn't need to) if you use a
> VLA or alloca.
>
> I'm considering applying the attached patch, which would make it so
> VLAs don't break thumb syscalls and eliminate the need to force frame
> pointer off when building as thumb. This is all a workaround for gcc
> being wrong about not letting you use r7, but it seems reasonable and
> non-invasive. It just omits r7 from the constraints and uses a temp
> register to save/restore it.

This seems to fail when compiling src/thread/arm/__set_thread_area.c:

  {standard input}: Assembler messages:
  {standard input}:45: Error: invalid constant (f0005) after fixup
  make: *** [obj/src/thread/arm/__set_thread_area.o] Error 1

Without the patch, __set_thread_area() effectively compiles to:

__set_thread_area:
    push    {r7, lr}
    ldr    r7, .L2
    pop    {r7, pc}
.L2:
    .word    983045

With the patch:

__set_thread_area:
    push    {r7, lr}
    add    r7, sp, #0
    mov r3,r7 ; mov r7,#983045 ; svc 0 ; mov r7,r3
    pop    {r7, pc}

ie the immediate value 0xf0005 can't be loaded directly into r7 with a
single Thumb2 mov instruction.

I tried a quick test to replace the single mov instruction in
__asm_syscall() with a movw + movt pair:

#define __asm_syscall(...) do { \
    __asm__ __volatile__ ( "mov %1,r7 ; movw r7,%2 & 0xffff ; movt
r7,%2 >> 16 ; svc 0 ; mov r7,%1" \
    : "=r"(r0), "=&r"((int){0}) : __VA_ARGS__ : "memory"); \
    return r0; \
    } while (0)

It fixes __set_thread_area() but causes a new error in syscall() as
the syscall number is passed to __asm_syscall() as a variable rather
than an immediate:

  {standard input}: Assembler messages:
  {standard input}:75: Error: constant expression expected -- `movw
r7,r6&0xffff'
  {standard input}:75: Error: constant expression expected -- `movt r7,r6>>16'
  make: *** [obj/src/misc/syscall.o] Error 1

Powered by blists - more mailing lists

Your e-mail address:

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.