From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Guy Briggs Subject: Re: [PATCH 2/2] audit: restore AUDIT_LOGINUID unset ABI Date: Tue, 16 Dec 2014 14:20:04 -0500 Message-ID: <20141216192004.GA17508@madcap2.tricolour.ca> References: <1560037.8CbAgy6kZ9@sifl> <20141212164450.GN29175@madcap2.tricolour.ca> <3032508.rvOTiD3vhV@sifl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <3032508.rvOTiD3vhV@sifl> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Paul Moore Cc: linux-audit@redhat.com, ebiederm@xmission.com, eparis@parisplace.org List-Id: linux-audit@redhat.com On 14/12/12, Paul Moore wrote: > On Friday, December 12, 2014 11:44:50 AM Richard Guy Briggs wrote: > > On 14/12/12, Paul Moore wrote: > > > On Friday, December 12, 2014 12:20:16 AM Richard Guy Briggs wrote: > > ... > > > > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > > > > index fb4d2df..ea62c7b 100644 > > > > --- a/kernel/auditfilter.c > > > > +++ b/kernel/auditfilter.c > > > > @@ -441,6 +441,7 @@ static struct audit_entry > > > > *audit_data_to_entry(struct > > > > audit_rule_data *data, if ((f->type == AUDIT_LOGINUID) && (f->val == > > > > AUDIT_UID_UNSET)) { f->type = AUDIT_LOGINUID_SET; > > > > > > > > f->val = 0; > > > > > > > > + entry->rule.flags |= AUDIT_LOGINUID_LEGACY; > > > > > > > > } > > > > > > > > if ((f->type == AUDIT_PID) || (f->type == AUDIT_PPID)) { > > > > > > > > @@ -592,7 +593,7 @@ static struct audit_rule_data > > > > *audit_krule_to_data(struct audit_krule *krule) return NULL; > > > > > > > > memset(data, 0, sizeof(*data)); > > > > > > > > - data->flags = krule->flags | krule->listnr; > > > > + data->flags = (krule->flags & ~AUDIT_LOGINUID_LEGACY) | > > > > krule->listnr; > > > > > > Argh! I missed that the audit_krule->flags end up in > > > audit_rule_data->flags. > > > > Well, it came in that way... > > Yes, it does, my mistake. I was probably just looking at the structure > definition, saw it wasn't exported to userspace, and thought the "flags" field > seemed promising. > > > > Bummer. > > > > > > Some thoughts: > > > > > > * Your 1/2 patch saved 32-bits in audit_krule, what are your thoughts on > > > adding a new 32-bit bitmap, say "private", which could be used internally > > > to track things like this? I'm not a big fan of overloading parts of the > > > public API for use by internal mechanisms, it almost always gets messy. > > > > I thought it was going to be messier, but I like how it turned out > > cleaner because of the way it was already used. > > Yes, I think using audit_krule->flags is an improvement over the previous > patch, but I think we are better served using a field that doesn't interfere > with the userspace API. But it doesn't interfere. A field is passed in from userspace that is multiplexed and has to be filtered anyways into its components in the internal representation. It is then combined again on output to userspace from more than one source. Arguably, the field passed in from userspace it mis-named, since it isn't strictly flags, but a list number from zero to five with a flag added just larger than any of the list numbers. The userspace API is not affected. Would you prefer if I filter the internal flags field with "& AUDIT_FILTER_PREPEND" rather than "& ~AUDIT_LOGINUID_LEGACY" to make it more clear what is being recombined in audit_krule_to_data()? > > > * Also, why is there both an audit_krule->flags and audit_krule->listnr > > > field? With the exception of the AUDIT_FILTER_PREPEND bit are they always > > > going to be the same? I wonder if some more cleanup could be done here > > > ... > > > > This is part of the API. The flags field is used to hand in the list > > number and its intended position on the list. Once it gets transferred > > from a user data blob to a kernel entry, it is split into listnr and > > flags. > > The question I was trying to ask, perhaps rhetorically at this point, is if > there is much/any advantage to spliting the public API flags into the private > flags/listnr field. It's probably not worth worrying about in the context of > this fix, just something that popped into my head when looking at this fix. > In retrospect I probably shouldn't have muddled the discussion with this idea. Ok, I hadn't understood that you were contemplating leaving that field passed from userspace as it was passed in, in the internal representation and simply filtering it for necessary information at the point of use. That has some merit, but that listnr value is used at least a dozen and a half places and would need filtering in each of those, slightly complicating/obfuscating the code. > > I thought it made sense to internally add it to the flags field. > > I would still like us to use an internal field for tracking things > that aren't part of the API. It *is* and internal field. Internally, the "flags" field is only used for *one* bit, which seems like a waste to add another 32-bit field to accomodate another single-bit flag. Is it worth turning it into a bitfield to avoid the confusion? The overhead of doing the filtering on rule creation and deletion seems pretty minimal compared to adding a 32-bit field that stays with every entry. > paul moore - RGB -- Richard Guy Briggs Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545