GNU bug report logs - #30918
mv: don't use syscall() to call renameat2()

Previous Next

To reply to this bug, email your comments to 30918 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ross Burton <ross <at> burtonini.com>
To: bug-coreutils <at> gnu.org, clint <at> debian.org
Subject: Don't use syscall() to call renameat2()
Date: Fri, 23 Mar 2018 17:38:25 +0000

[Message part 1 (text/plain, inline)]

mv.c uses gnulib/renameat2.c to call renameat2(), which if the glibc
wrapper isn't available will just invoke syscall(SYS_renameat2). This may
seem like a good idea but considering a number of major distributions use
LD_PRELOAD to build as a pretend root user[1] these mv calls won't be
intercepted, and building will break in strange and interesting ways (such
as binaries not being owned by root:root anymore).  Please consider
changing renameat2.c so that it doesn't hit syscall() if the wrapper isn't
available.

Ross

[1] Debian and derivatives using fakeroot, OpenEmbedded derivatives using
pseudo, etc.

[Message part 2 (text/html, inline)]

Message #8 received at 30918 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Ross Burton <ross <at> burtonini.com>, 30918 <at> debbugs.gnu.org, clint <at> debian.org
Subject: Re: bug#30918: Don't use syscall() to call renameat2()
Date: Fri, 23 Mar 2018 12:02:36 -0700

On 03/23/2018 10:38 AM, Ross Burton wrote:
> Please consider
> changing renameat2.c so that it doesn't hit syscall() if the wrapper isn't
> available.

That would reintroduce race-condition security holes in the ordinary 
build of GNU Coreutils on GNU/Linux, which would not be a good thing. 
Instead, how about fixing fakeroot so that it traps 'syscall' and fails 
with errno == ENOTSUP? Better yet, fix fakeroot so that it implements 
the renameat2 semantics with that syscall. (Or even better, add 
renameat2 to both glibc and fakeroot. :-)

Message #11 received at 30918 <at> debbugs.gnu.org (full text, mbox):

From: Clint Adams <clint <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 30918 <at> debbugs.gnu.org, Ross Burton <ross <at> burtonini.com>
Subject: Re: bug#30918: Don't use syscall() to call renameat2()
Date: Sat, 24 Mar 2018 21:00:24 +0000

On Fri, Mar 23, 2018 at 12:02:36PM -0700, Paul Eggert wrote:
> That would reintroduce race-condition security holes in the ordinary build
> of GNU Coreutils on GNU/Linux, which would not be a good thing. Instead, how
> about fixing fakeroot so that it traps 'syscall' and fails with errno ==
> ENOTSUP? Better yet, fix fakeroot so that it implements the renameat2
> semantics with that syscall. (Or even better, add renameat2 to both glibc
> and fakeroot. :-)

What's keeping it out of glibc?

Message #14 received at 30918 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Clint Adams <clint <at> gnu.org>
Cc: 30918 <at> debbugs.gnu.org, Ross Burton <ross <at> burtonini.com>
Subject: Re: bug#30918: Don't use syscall() to call renameat2()
Date: Sat, 24 Mar 2018 14:06:30 -0700

Clint Adams wrote:
> What's keeping it out of glibc?

Sorry, don't know offhand. Mostly lack of time, I expect.

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Richard Purdie <richard.purdie <at> linuxfoundation.org>
To: eggert <at> cs.ucla.edu, bug-coreutils <at> gnu.org, Burton Ross
 <ross.burton <at> intel.com>, Seebs <seebs <at> seebs.net>
Subject: bug#30918: Don't use syscall() to call renameat2()
Date: Sun, 25 Mar 2018 11:15:48 +0100

> On 03/23/2018 10:38 AM, Ross Burton wrote:
> > Please consider changing renameat2.c so that it doesn't hit 
> > syscall() if the wrapper isn't available.
> 
> That would reintroduce race-condition security holes in the ordinary 
> build of GNU Coreutils on GNU/Linux, which would not be a good 
> thing. Instead, how about fixing fakeroot so that it traps 'syscall' 
> and fails with errno == ENOTSUP? Better yet, fix fakeroot so that it 
> implements the renameat2 semantics with that syscall. (Or even 
> better, add renameat2 to both glibc and fakeroot. :-)

I've just had a look at this situation and its not as simple as it may
first appear. The function prototype for syscall() in posix/unistd.h
is:

extern long int syscall (long int __sysno, ...)

and the implementation in glibc is in assembler for each architecture.
The syscall(2) man page also gives a little bit more of a hint of the
challenges in the syscall() function with register splits and alignment
along with different forms of error handling. You can call it with
varying numbers of options and the register usage needs to be tightly
controlled, its not a "normal" function where standard function calling
conventions will always work.

So yes, we could add a wrapper in pseudo however we're likely going to
have to end up using assembler to avoid smashing the calling stack in
the general case. That would be on a per architecture basis and comes
with all the complexities that brings.

I'd therefore like to add my own plea to figure out and use some glibc
API for this even if we have to establish it.

Cheers,

Richard

Message #20 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Seebs <seebs <at> seebs.net>
To: Richard Purdie <richard.purdie <at> linuxfoundation.org>
Cc: eggert <at> cs.ucla.edu, bug-coreutils <at> gnu.org,
 Burton Ross <ross.burton <at> intel.com>
Subject: Re: bug#30918: Don't use syscall() to call renameat2()
Date: Sun, 25 Mar 2018 09:37:06 -0500

On Sun, 25 Mar 2018 11:15:48 +0100
Richard Purdie <richard.purdie <at> linuxfoundation.org> wrote:

> > On 03/23/2018 10:38 AM, Ross Burton wrote:
> > > Please consider changing renameat2.c so that it doesn't hit 
> > > syscall() if the wrapper isn't available.
> > 
> > That would reintroduce race-condition security holes in the
> > ordinary build of GNU Coreutils on GNU/Linux, which would not be a
> > good thing. Instead, how about fixing fakeroot so that it traps
> > 'syscall' and fails with errno == ENOTSUP? Better yet, fix fakeroot
> > so that it implements the renameat2 semantics with that syscall.
> > (Or even better, add renameat2 to both glibc and fakeroot. :-)
> 
> I've just had a look at this situation and its not as simple as it may
> first appear. The function prototype for syscall() in posix/unistd.h
> is:
> 
> extern long int syscall (long int __sysno, ...)
> 
> and the implementation in glibc is in assembler for each architecture.
> The syscall(2) man page also gives a little bit more of a hint of the
> challenges in the syscall() function with register splits and
> alignment along with different forms of error handling. You can call
> it with varying numbers of options and the register usage needs to be
> tightly controlled, its not a "normal" function where standard
> function calling conventions will always work.
> 
> So yes, we could add a wrapper in pseudo however we're likely going to
> have to end up using assembler to avoid smashing the calling stack in
> the general case. That would be on a per architecture basis and comes
> with all the complexities that brings.
> 
> I'd therefore like to add my own plea to figure out and use some glibc
> API for this even if we have to establish it.

I have significant concerns about the feasibility of a generic wrapper
for syscall(). In particular, a "wrapper" which does *nothing* but
forward arguments may well be practical. Or one which just fails
immediately and claims ENOTSUPP -- but this creates the risk that we'll
break things which were using perfectly valid syscalls which work fine
and which we don't need to intercept or do anything with.

But if we don't want to break code which is using syscall() for other
operations, we would have to (1) successfully forward all system calls
*and* handle their returns, (2) also intercept specific cases and
modify their parameters. Which requires us to *comprehend* their
parameters. For instance, in the pseudo environment, we may be
virtualizing a chroot() operation, so a literal renameat2() argument of
"/a" gets translated into "/chroot/path/a" before it gets handed to the
kernel.

Take a look at the man page for syscall(2), and consider what we have
to do if we want to *handle* the arguments in any way. For instance, if
we needed to intercept SYS_readahead on EABI (we wouldn't, but it's
the example they give in the man page), we'd have to process arguments
completely differently from if we were processing it on x86. I am not
sure whether there's parallel concerns for 64-bit pointers on a 64-bit
ARM system. I would also have concerns about the "sets registers to
indicate success" behavior; wrapper functions are going to make *other
system calls* after calling the underlying syscall, so things like that
could (and in that case, I think probably would) get smashed by the
later syscalls.

I'm assuming the race condition refers to the behavior of
RENAME_EXCHANGE. I hadn't seen that before, and I don't know of an
existing mv(1) usage which would use it, but it does seem an
exceptionally desireable thing to have available. On the other hand,
I'm not sure it's technically *possible* to fix this in pseudo.
(I'm aware that pseudo as a whole is well past the realm of "merely
undefined" behavior and into "why would you do that, what's wrong with
you", but we haven't been able to make the requirement go away.)

I will be adding a wrapper for renameat2() to pseudo, but I can't make
glibc change its behavior so quickly.

(And now that I look more closely at the flags, supporting
RENAME_EXCHANGE will require more complicated effort than I'd initially
realized.)

-s

Message #23 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Seebs <seebs <at> seebs.net>,
 Richard Purdie <richard.purdie <at> linuxfoundation.org>
Cc: bug-coreutils <at> gnu.org, Burton Ross <ross.burton <at> intel.com>
Subject: Re: bug#30918: Don't use syscall() to call renameat2()
Date: Sun, 25 Mar 2018 18:20:56 -0700

Seebs wrote:
> I have significant concerns about the feasibility of a generic wrapper
> for syscall(). In particular, a "wrapper" which does *nothing* but
> forward arguments may well be practical. Or one which just fails
> immediately and claims ENOTSUPP -- but this creates the risk that we'll
> break things which were using perfectly valid syscalls which work fine
> and which we don't need to intercept or do anything with.

For this particular issue, failing with ENOTSUPP should do. Perhaps such a 
behavior could be available as a link-time or runtime option.

More precise would be to have syscall to do nothing but forward arguments, 
*except* for the renameat2 syscall which would work much like the renameat 
wrapper that I assume you already have. This would work for coreutils, shouldn't 
break anything else, shouldn't require a link-time or runtime option, and 
shouldn't be that much harder than always forwarding syscall arguments.

> I'm assuming the race condition refers to the behavior of
> RENAME_EXCHANGE.

No, it's RENAME_NOREPLACE. Coreutils doesn't use RENAME_EXCHANGE now (though it 
might in the future, I suppose).

Message #26 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Seebs <seebs <at> seebs.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Richard Purdie <richard.purdie <at> linuxfoundation.org>, bug-coreutils <at> gnu.org,
 Burton Ross <ross.burton <at> intel.com>
Subject: Re: bug#30918: Don't use syscall() to call renameat2()
Date: Sun, 25 Mar 2018 20:52:32 -0500

On Sun, 25 Mar 2018 18:20:56 -0700
Paul Eggert <eggert <at> cs.ucla.edu> wrote:

> Seebs wrote:
> > I have significant concerns about the feasibility of a generic
> > wrapper for syscall(). In particular, a "wrapper" which does
> > *nothing* but forward arguments may well be practical. Or one which
> > just fails immediately and claims ENOTSUPP -- but this creates the
> > risk that we'll break things which were using perfectly valid
> > syscalls which work fine and which we don't need to intercept or do
> > anything with.
> 
> For this particular issue, failing with ENOTSUPP should do. Perhaps
> such a behavior could be available as a link-time or runtime option.
> 
> More precise would be to have syscall to do nothing but forward
> arguments, *except* for the renameat2 syscall which would work much
> like the renameat wrapper that I assume you already have. This would
> work for coreutils, shouldn't break anything else, shouldn't require
> a link-time or runtime option, and shouldn't be that much harder than
> always forwarding syscall arguments.

We actually hadn't added a wrapper for it; so far as I can tell, no
one's ever used it before. (Or at least, no one's ever used it in such
a way that failing to catch it was causing detectable issues.) It's on
my list now that I've seen an actual program attempting to use it.

And I don't think it's that simple; as noted, I'm not convinced that
a function written in C can reliably interpret the arguments of some
syscalls across targets. (renameat2 may not be one of those affected.)
For instance, as noted in the man page, if I wanted to try to interpret
the arguments passed to SYS_readahead, I'd have to do things differently
for EABI ARM than I would for x86. That's a degree of additional magic
not previously present in any of the wrappers, perhaps surprisingly.

I haven't got a MIPS machine handy to go look, but the kind of thing
I'm concerned about is the description from syscall(2):

>       On a few architectures, a register is used to indicate simple
>       boolean failure of the system call:  ia64 uses r10 for this
>       purpose, and mips uses a3.

If I'm writing a wrapper in C, I can't preserve that value, but I have
to make *other* system calls before and after calling the underlying
wrapper.

So if it's the case that, after a call into syscall(), some value has
been stored in register a3 on MIPS... There's nothing I can write in
C that will preserve that value for my caller, and the other system
calls I make after the call to the "real" syscall() may overwrite it.
So the caller will get the wrong value, and if they were assuming that
syscall() would perform as expected...

This function may actually be Too Magic to sanely wrap. I think this is
the only library function I've ever seen document a need to insert an
unused argument between two arguments, but only for a specific ABI.

(We do have a couple of arch-specific hooks, but they're at the level
of "which compatibility version to specify for a particular function".)

-s

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.