linux-kernel - Re: [kernel-hardening] [PATCH 3/4] Make static usermode helper binaries constant

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 15 Dec 2016 09:54:39 -0800
From:   Greg KH <greg@...ah.com>
To:     kernel-hardening@...ts.openwall.com
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [kernel-hardening] [PATCH 3/4] Make static usermode helper
 binaries constant

On Wed, Dec 14, 2016 at 12:54:44PM -0800, Greg KH wrote:
> On Wed, Dec 14, 2016 at 03:29:52PM -0500, Rich Felker wrote:
> > On Wed, Dec 14, 2016 at 10:50:52AM -0800, Greg KH wrote:
> > > 
> > > There are a number of usermode helper binaries that are "hard coded" in
> > > the kernel today, so mark them as "const" to make it harder for someone
> > > to change where the variables point to.
> > 
> > You're not preventing change of where they point to, but rather
> > preventing modification of the pointed-to data through these
> > pointers...
> > 
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > > ---
> > >  drivers/macintosh/windfarm_core.c          | 2 +-
> > >  drivers/net/hamradio/baycom_epp.c          | 2 +-
> > >  drivers/pnp/pnpbios/core.c                 | 5 +++--
> > >  drivers/staging/greybus/svc_watchdog.c     | 4 ++--
> > >  drivers/staging/rtl8192e/rtl8192e/rtl_dm.c | 6 +++---
> > >  fs/nfsd/nfs4layouts.c                      | 6 ++++--
> > >  security/keys/request_key.c                | 7 ++++---
> > >  7 files changed, 18 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
> > > index 465d770ab0bb..1b317cbb73cf 100644
> > > --- a/drivers/macintosh/windfarm_core.c
> > > +++ b/drivers/macintosh/windfarm_core.c
> > > @@ -74,7 +74,7 @@ static inline void wf_notify(int event, void *param)
> > >  
> > >  static int wf_critical_overtemp(void)
> > >  {
> > > -	static char * critical_overtemp_path = "/sbin/critical_overtemp";
> > > +	static const char * critical_overtemp_path = "/sbin/critical_overtemp";
> > 
> > Should be static char *const critical_overtemp_path, or if you prefer
> > static const char *const critical_overtemp_path (since the pointed-to
> > data is not modifiable either). Likewise elsewhere.
> 
> argh, ok, I failed here, thanks for that, that's what I get for writing
> code on an airplane...
> 
> let me rework this, I also want to make argv and env static too, just
> for good measure.

To follow up on this, and after staring at too many outputs of the
compiler, I think what this really should be is:
	static char const critical_overtemp_path[] = "/sbin/critical_overtemp";
right?

That way both the variable, and the data, end up in read-only memory
from what I can tell.

But, if I do:
	static char const char critical_overtemp_path[] = "/sbin/critical_overtemp";
then sparse complains to me about:
	warning: duplicate const

Is that just sparse being dense, or is the latter one really better
here?  It seems that both of the above put the data and variable into
the same segment (.rodata).

thanks,

greg k-h