[RFC PATCH] audit: fix a race condition with the auditd tracking code

[Paul Moore]

On Thu, Jun 15, 2017 at 3:24 PM, Richard Guy Briggs <rgb at redhat.com> wrote:
> On 2017-06-15 11:28, Paul Moore wrote:
>> From: Paul Moore <paul at paul-moore.com>
>>
>> Originally reported by Adam and Dusty, it appears we have a small
>> race window in kauditd_thread(), as documented in the Fedora BZ:
>>
>>  * https://bugzilla.redhat.com/show_bug.cgi?id=1459326#c35
>>
>>  "This issue is partly due to the read-copy nature of RCU, and
>>   partly due to how we sync the auditd_connection state across
>>   kauditd_thread and the audit control channel.  The kauditd_thread
>>   thread is always running so it can service the record queues and
>>   emit the multicast messages, if it happens to be just past the
>>   "main_queue" label, but before the "if (sk == NULL || ...)"
>>   if-statement which calls auditd_reset() when the new auditd
>>   connection is registered it could end up resetting the auditd
>>   connection, regardless of if it is valid or not.  This is a rather
>>   small window and the variable nature of multi-core scheduling
>>   explains why this is proving rather difficult to reproduce."
>>
>> The fix is to have functions only call auditd_reset() when they
>> believe that the kernel/auditd connection is still valid, e.g.
>> non-NULL, and to have these callers pass their local copy of the
>> auditd_connection pointer to auditd_reset() where it can be compared
>> with the current connection state before resetting.  If the caller
>> has a stale state tracking pointer then the reset is ignored.
>>
>> We also make a small change to kauditd_thread() so that if the
>> kernel/auditd connection is dead we skip the retry queue and send the
>> records straight to the hold queue.  This is necessary as we used to
>> rely on auditd_reset() to occasionally purge the retry queue but we
>> are going to be calling the reset function much less now and we want
>> to make sure the retry queue doesn't grow unbounded.
>>
>> Reported-by: Adam Williamson <awilliam at redhat.com>
>> Reported-by: Dusty Mabe <dustymabe at redhat.com>
>> Signed-off-by: Paul Moore <paul at paul-moore.com>
>
> This looks reasonable.
> Reviewed-by: Richard Guy Briggs <rgb at redhat.com>

Thanks for the review.  Similar to the other patch, I'll merge this
tomorrow if nothing pops up.

(Yes, it's slightly more complicated than the other patch, but it's a
relatively low risk bug-fix so it warrants merging at this late stage
I think.)

-- 
paul moore
www.paul-moore.com