Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 10 Jun 2018 00:41:59 +0200
From: Szabolcs Nagy <nsz@...t70.net>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 0/9] linux v4.16 update

* Rich Felker <dalias@...c.org> [2018-05-15 13:39:23 -0400]:
> On Sat, Apr 28, 2018 at 09:56:56PM +0200, Szabolcs Nagy wrote:
> > From f7eb8934396890e39b9e5e2b4fdd1534f1c024cc Mon Sep 17 00:00:00 2001
> > From: Szabolcs Nagy <nsz@...t70.net>
> > Date: Mon, 16 Apr 2018 22:16:29 +0000
> > Subject: [PATCH 1/9] siginfo struct change following linux v4.16
> > 
> > this is supposed to be a cosmetic change only, not affecting api or abi,
> > follows linux commit b68a68d3dcc15ebbf23cbe91af1abf57591bd96b and
> > 859d880cf544dbe095ce97534ef04cd88ba2f2b4 with slightly different field
> > names to follow musl conventions (which must be different to avoid
> > depending on anonymous union support and polluting the namespace).
> > ---
> >  include/signal.h | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/signal.h b/include/signal.h
> > index a4f85cca..d68331e8 100644
> > --- a/include/signal.h
> > +++ b/include/signal.h
> > @@ -123,13 +123,17 @@ typedef struct {
> >  		} __si_common;
> >  		struct {
> >  			void *si_addr;
> > -			short si_addr_lsb;
> >  			union {
> > +				short si_addr_lsb;
> >  				struct {
> > +					void *__dummy_bnd;
> >  					void *si_lower;
> >  					void *si_upper;
> >  				} __addr_bnd;
> > -				unsigned si_pkey;
> > +				struct {
> > +					void *__dummy_pkey;
> > +					unsigned si_pkey;
> > +				} __addr_pkey;
> >  			} __first;
> >  		} __sigfault;
> >  		struct {
> > @@ -150,10 +154,10 @@ typedef struct {
> >  #define si_stime   __si_fields.__si_common.__second.__sigchld.si_stime
> >  #define si_value   __si_fields.__si_common.__second.si_value
> >  #define si_addr    __si_fields.__sigfault.si_addr
> > -#define si_addr_lsb __si_fields.__sigfault.si_addr_lsb
> > +#define si_addr_lsb __si_fields.__sigfault.__first.si_addr_lsb
> >  #define si_lower   __si_fields.__sigfault.__first.__addr_bnd.si_lower
> >  #define si_upper   __si_fields.__sigfault.__first.__addr_bnd.si_upper
> > -#define si_pkey    __si_fields.__sigfault.__first.si_pkey
> > +#define si_pkey    __si_fields.__sigfault.__first.__addr_pkey.si_pkey
> >  #define si_band    __si_fields.__sigpoll.si_band
> >  #define si_fd      __si_fields.__sigpoll.si_fd
> >  #define si_timerid __si_fields.__si_common.__first.__timer.si_timerid
> 
> Is there any benefit to making this change?
> 

only to follow the linux headers might make it easier to track changes

turns out the change was wrong though and now it's even more messy:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8420f71943ae96dcd78da5bd4a5c2827419d340c


> > From de4ca3284d759563322c795479601e9eee394832 Mon Sep 17 00:00:00 2001
> > From: Szabolcs Nagy <nsz@...t70.net>
> > Date: Sat, 28 Apr 2018 17:25:41 +0000
> > Subject: [PATCH 9/9] [RFC] add new memory mapping related apis
> > 
> > memfd_create (linux v3.17)
> > mlock2 (linux v4.4)
> > pkey_alloc (linux v4.9)
> > pkey_free (linux v4.9)
> > pkey_mprotect (linux v4.9)
> > pkey_get (glibc 2.27)
> > pkey_set (glibc 2.27)
> > 
> > notes:
> > 
> > - pkey_alloc type is inconsistent between the manual and
> >   the glibc source (unsigned int vs unsigned long args)
> 
> This needs to be resolved, I think.
> 

glibc is right, the man was documenting the syscall layer

> > - pkey_get / pkey_set are not syscalls (query/update the
> >   pkru register on x86), not implemented yet.
> > - fallback in mlock2 to mlock and pkey_mprotect to mprotect
> >   follows glibc.
> > - mlock2 EINVAL return means invalid (or unsupported) flags.
> > - moved MLOCK_ONFAULT under _GNU_SOURCE following glibc.
> > ---
> >  arch/powerpc/bits/mman.h   |  4 ++++
> >  arch/powerpc64/bits/mman.h |  4 ++++
> >  include/sys/mman.h         | 24 +++++++++++++++++++++---
> >  src/linux/memfd_create.c   | 10 ++++++++++
> >  src/linux/mlock2.c         | 15 +++++++++++++++
> >  src/linux/pkey_alloc.c     | 15 +++++++++++++++
> >  src/linux/pkey_get.c       |  9 +++++++++
> >  src/linux/pkey_mprotect.c  | 15 +++++++++++++++
> >  src/linux/pkey_set.c       |  9 +++++++++
> >  9 files changed, 102 insertions(+), 3 deletions(-)
> 
> Am I correct in assuming the header & external function interfaces
> match how glibc is exposing these functions?
> 
> One thing that looks a little bit weird to me is conditioning
> existence of some functions on the syscall macro:
> 
> > diff --git a/src/linux/pkey_alloc.c b/src/linux/pkey_alloc.c
> > new file mode 100644
> > index 00000000..0b0770a6
> > --- /dev/null
> > +++ b/src/linux/pkey_alloc.c
> > @@ -0,0 +1,15 @@
> > +#include "syscall.h"
> > +
> > +#ifdef SYS_pkey_alloc
> > +#define _GNU_SOURCE 1
> > +#include <sys/mman.h>
> > +int pkey_alloc(unsigned flags, unsigned access)
> > +{
> > +	return syscall(SYS_pkey_alloc, flags, access);
> > +}
> > +
> > +int pkey_free(int pkey)
> > +{
> > +	return syscall(SYS_pkey_free, pkey);
> > +}
> > +#endif
> 
> where others fail with ENOSYS when the arch has no definition. I think
> it's also problematic that syscall.h is being included before #define
> _GNU_SOURCE. This would break under changes in how the features.h
> mechanisms work.
> 

ok this should fail ENOSYS when not available.

> Is there a reason you want to avoid an external functon existing when
> the arch lacks the feature?
> 
> The other patches in this series look ok I think. I'll commit them
> soon if I don't see any further things to ask about.
> 
> Rich

Powered by blists - more mailing lists

Your e-mail address:

Powered by Openwall GNU/*/Linux - Powered by OpenVZ