Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

On 2018-07-19 18:47, Paul Moore wrote: > On Thu, Jul 19, 2018 at 12:10 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > On 2018-07-18 17:45, Paul Moore wrote: > > > On Thu, Jul 12, 2018 at 8:43 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > > > On 2018-06-28 15:41, Paul Moore wrote: > > > > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > > > > > The failure to add an audit rule due to audit locked gives no clue > > > > > > what CONFIG_CHANGE operation failed. > > > > > > Similarly the set operation is the only other operation that doesn't > > > > > > give the "op=" field to indicate the action. > > > > > > All other CONFIG_CHANGE records include an op= field to give a clue as > > > > > > to what sort of configuration change is being executed. > > > > > > > > > > > > Since these are the only CONFIG_CHANGE records that that do not have an > > > > > > op= field, add them to bring them in line with the rest. > > > > > > > > > > Normally this would be an immediate reject because this patch inserts > > > > > a field into an existing record, but the CONFIG_CHANGE record is so > > > > > variable (supposedly bad in its own right) that I don't this really > > > > > matters. > > > > > > > > > > With that out of the way, I think this patch is fine, but I don't > > > > > think it is complete. At the very least there is another > > > > > CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't > > > > > appear to include an "op" field. If we want to make sure we have an > > > > > "op" field in every CONFIG_CHANGE record, let's actually add them all > > > > > :) > > > > > > > > The version I'm looking at already had it when it was added in 2009. > > > > > > Yup, there it is ... now I'm wondering what tree I was looking at as a > > > reference while reviewing this? > > > > > > /me scratches head > > > > > > > This one doesn't add the auid and ses fields because they will be > > > > covered by the linking of this record with the syscall record via the > > > > audit_context() introduced in another patch. > > > > > > Yeah, I'm not concerned about that for the reasons you state. > > > > > > > > and one more in audit_receive_msg(). There may be more. > > > > > > > > I believe they're covered by other patches in the ghak59 set. > > > > > > If they are in the later patches it might be good to move those "op=" > > > additions into this patch. > > > > I don't see any CONFIG_CHANGE records generated in audit_receive_msg() > > that are missing op= field. Can you narrow it down? > > Well, just grep'ing my way through audit_receive_msg() I see that > AUDIT_ADD/DEL_RULE generates a CONFIG_CHANGE record. The failure case is addressed in this patch. The success case is addressed in audit_log_rule_change(). The latter already has it. What is the problem? What tree are you looking at? What am I missing?