From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Moore Subject: Re: [PATCH 2/2] audit: restore AUDIT_LOGINUID unset ABI Date: Fri, 12 Dec 2014 14:23:53 -0500 Message-ID: <3032508.rvOTiD3vhV@sifl> References: <1560037.8CbAgy6kZ9@sifl> <20141212164450.GN29175@madcap2.tricolour.ca> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20141212164450.GN29175@madcap2.tricolour.ca> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Richard Guy Briggs Cc: linux-audit@redhat.com, ebiederm@xmission.com, eparis@parisplace.org List-Id: linux-audit@redhat.com 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. > > * 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. > 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. -- paul moore security and virtualization @ redhat