[PATCH 3/6] audit: Fix possible tagging failures

[Amir Goldstein]

On Tue, Jul 3, 2018 at 5:21 PM, Jan Kara <jack at suse.cz> wrote:
> On Fri 29-06-18 15:05:07, Amir Goldstein wrote:
>> On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack at suse.cz> wrote:
>> > Audit tree code is replacing marks attached to inodes in non-atomic way.
>> > Thus fsnotify_find_mark() in tag_chunk() may find a mark that belongs to
>> > a chunk that is no longer valid one and will soon be destroyed. Tags
>> > added to such chunk will be simply lost.
>> >
>> > Fix the problem by making sure old mark is marked as going away (through
>> > fsnotify_detach_mark()) before dropping mark_mutex and thus in an atomic
>> > way wrt tag_chunk(). Note that this does not fix the problem completely
>> > as if tag_chunk() finds a mark that is going away, it fails with
>> > -ENOENT. But at least the failure is not silent and currently there's no
>> > way to search for another fsnotify mark attached to the inode. We'll fix
>> > this problem in later patch.
>> >
>> > Signed-off-by: Jan Kara <jack at suse.cz>
>> > ---
>>
>> This one too looks sane.
>> Without knowing anything about audit_watch, there seems to be
>> an fsnotify_destroy_mark() after unlock of audit_filter_mutex, so it
>> may require a similar fix.
>
> Where? I don't see any call to fsnotify_destroy_mark() left after this
> patch...
>

Not directly related to this cleanup, but looking in other audit modules,
fsnotify_destroy_mark() in audit_remove_parent_watches() is called
outside audit_filter_mutex and audit_find_parent() in audit_add_watch()
is called inside audit_filter_mutex, so I was wondering if there was a
similar race window in that code. I didn't spend enough time looking
at audit_watch.c to understand what is going on in there.

Thanks,
Amir.