From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755153AbbHCSuo (ORCPT ); Mon, 3 Aug 2015 14:50:44 -0400 Received: from wtarreau.pck.nerim.net ([62.212.114.60]:35556 "EHLO 1wt.eu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754099AbbHCSun (ORCPT ); Mon, 3 Aug 2015 14:50:43 -0400 Date: Mon, 3 Aug 2015 20:50:13 +0200 From: Willy Tarreau To: Andy Lutomirski Cc: Andy Lutomirski , Kees Cook , Steven Rostedt , "security@kernel.org" , X86 ML , Borislav Petkov , Sasha Levin , LKML , Konrad Rzeszutek Wilk , Boris Ostrovsky , Andrew Cooper , Jan Beulich , xen-devel Subject: Re: [PATCH 1/2] sysctl: add a new generic strategy to make permanent changes on negative values Message-ID: <20150803185013.GA24014@1wt.eu> References: <1438626217-23970-1-git-send-email-w@1wt.eu> <1438626217-23970-2-git-send-email-w@1wt.eu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 03, 2015 at 11:33:30AM -0700, Andy Lutomirski wrote: > On Mon, Aug 3, 2015 at 11:23 AM, Willy Tarreau wrote: > > The new function is proc_dointvec_minmax_negperm(), it refuses to change > > the value if the current one is already negative. This will be used to > > lock down some settings such as sensitive system calls. > > > > Signed-off-by: Willy Tarreau > > --- > > kernel/sysctl.c | 36 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > > index 19b62b5..86c95a8 100644 > > --- a/kernel/sysctl.c > > +++ b/kernel/sysctl.c > > @@ -185,6 +185,9 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write, > > void __user *buffer, size_t *lenp, loff_t *ppos); > > #endif > > > > +static int proc_dointvec_minmax_negperm(struct ctl_table *table, int write, > > + void __user *buffer, size_t *lenp, loff_t *ppos); > > + > > static int proc_dointvec_minmax_coredump(struct ctl_table *table, int write, > > void __user *buffer, size_t *lenp, loff_t *ppos); > > #ifdef CONFIG_COREDUMP > > @@ -2249,6 +2252,33 @@ static void validate_coredump_safety(void) > > #endif > > } > > > > +/* Like minmax except that it refuses any change if the value was already > > + * negative. It silently ignores overrides with the same negative value. > > + */ > > +static int do_proc_dointvec_negperm_conv(bool *negp, unsigned long *lvalp, > > + int *valp, > > + int write, void *data) > > +{ > > + if (write && *valp < 0 && (!*negp || *valp != (int)*lvalp)) > > I could easily have failed to follow the bizarre negative sign > convention, but shouldn't that be "*valp != -(int)*lvalp" or similar? Not exactly since the sign is passed via negp apparently. There is an expression in the called function which first assigns lvalp or -lvalp to val depending on val, then uses the resulting value. The code above is the (simplified for me) equivalent of : int val = *negp ? -*lvalp : *lvalp; if (write && *valp < 0 && *valp != val) return -EINVAL; Maybe you find it more readable in which case I can redo it this way ? In my case it was the opposite in fact, I want to reject non-negative values as well as the negative ones not equal to *valp. Note that we could have decided to make it even simpler and always reject writes once *valp is < 0 but I find that it would be annoying for hardening scripts which would not be idempotent anymore. Willy