Re: [glibc PATCH] fcntl: put F_OFD_* constants under #ifdef __USE_FILE_OFFSET64

[Jeff Layton <jlayton@redhat.com>]

On Wed, 2016-08-17 at 20:02 +0200, Florian Weimer wrote:
> On 08/17/2016 07:39 PM, Jeff Layton wrote:
> > 
> > On Wed, 2016-08-17 at 19:34 +0200, Florian Weimer wrote:
> > > 
> > > On 08/17/2016 04:47 PM, Jeff Layton wrote:
> > > > 
> > > > 
> > > > The Linux kernel expects a flock64 structure whenever you use
> > > > OFD locks
> > > > with fcntl64. Unfortunately, you can currently build a 32-bit
> > > > program
> > > > that passes in a struct flock when it calls fcntl64.
> > > > 
> > > > Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is
> > > > also
> > > > defined, so that the build fails in this situation rather than
> > > > producing a broken binary.
> > > 
> > > Doesn't this affect legacy POSIX-style locks as well, under very
> > > similar
> > > circumstances?
> > > 
> > > 
> > 
> > No. The kernel will decide which type of struct it is based on
> > whether
> > userland passes in F_SETLK or F_SETLK64.
> 
> Let me see if I can sort this out.  Is the situation like this?
> 
>          _FILE_OFFSET_…    …BITS == 32          …BITS == 64
>          struct …       flock   flock64    flock   flock64
> fcntl (F_SETLK)        ok      BAD        ok      BAD
> fcntl (F_SETLK64)      BAD     ok         ok      ok
> fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok
> 
> ¹ is broken by your patch, right?

Not sure I 100% understand your chart, but if I do then I think it's
more like:

         _FILE_OFFSET_…    …BITS == 32          …BITS == 64
         struct …       flock   flock64    flock   flock64
fcntl (F_SETLK)        ok      BAD        ok      ok
fcntl (F_SETLK64)      BAD     ok         ok      ok
fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok

struct flock and struct flock64 are generally equivalent when
_FILE_OFFSET_BITS==64.

I don't quite understand how ¹ would be broken by this patch. The idea
with the patch is to ensure that if you haven't defined
_FILE_OFFSET_BITS=64 on a 32 bit arch, that it's broken at compile time
instead of at runtime.

> 
> Looking at the definition of struct flock and struct flock64, the
> risk 
> is that application silently succeed in locking the wrong thing when 
> using struct flock64 with a 32-it interface.
> 

Yes. The basic problem is that the kernel will expect a struct flock64,
but if you don't set _FILE_OFFSET_BITS=64 glibc will pass in a legacy
struct flock instead. The kernel can then read beyond the end of the
struct.

The bytes in l_start and l_len will be slurped into the kernel's
l_start field. The pid and whatever junk is beyond the struct will be
in the l_len and pid fields.

It's also possible the program will get back EFAULT as well if
copy_from_user fails.

-- 
Jeff Layton <jlayton@redhat.com>