Re: [BUG] fanotify: destroy/add race leaves a mark on a detached connector

[Jan Kara]

Hello!

On Thu 27-08-26 13:50:04, Daehyeon Ko wrote:
> I found an unprivileged race in fsnotify_destroy_marks() that can leave an
> attached fanotify mark on a detached connector. On unmodified Linux v7.2 and
> v6.12.105 source builds, it naturally reaches the fsnotify_conn_mask()
> warning from fanotify_mark(). On unmodified builds, the only observed
> system-wide failure was a v7.2 panic when panic_on_warn=1; no memory
> corruption or privilege escalation was observed.
> 
> Tested and source-inspected versions:
> 
>   - Linux v7.2, commit
>     8d3ae59288f1e7d58d76558a6ee96d533bc5019f: reproduced.
>   - Linux v6.12.105, commit
>     14c37ff05f22da2fa7076d10f6a07c7ede330c83: reproduced.
>   - Torvalds master at
>     73e3f0710014fe6d4ed98cfc02292f6121db7558: the relevant destroy
>     iterator and fdinfo consumer remain present. Commit e422777fdd47
>     changed mark-mask updates and can mask the immediate warning, but it
>     did not change this iterator or the detached-connector state.
> 
> The unconditional connector detachment involved in the race appears to
> originate in commit 6b3f05d24d35 ("fsnotify: Detach mark from object list
> when last reference is dropped"), released in v4.12-rc1. My unprivileged
> runtime results are limited to the two versions listed above. The limited
> unprivileged fanotify functionality used by this reproducer was introduced
> by 7cea2a3c505e ("fanotify: support limited functionality for unprivileged
> users"), released in v5.13-rc1; I am not asserting ordinary-user
> reachability before that change.
> 
> Root cause:
> 
> fsnotify_destroy_marks() holds a reference to the current mark, drops
> conn->lock, destroys that mark, and then continues the hlist walk. While the
> lock is dropped, an equal-priority mark can be inserted immediately before
> the current entry. The walk resumes from the current entry's next pointer,
> so it never visits the new predecessor, but it still detaches the connector
> from the object after the walk.
> 
> The skipped mark remains ALIVE|ATTACHED and remains on both its group list
> and the connector list. At the same time, the object no longer points to the
> connector, conn->obj is NULL, and conn->type is DETACHED.
> 
> Representative v6.12.105 output is:
> 
>   WARNING: CPU: 1 PID: 163 at fs/notify/mark.c:128
>   fsnotify_conn_mask+0x113/0x150
>   CPU: 1 UID: 65534 PID: 163 Comm: fanotify_destro
>   ...
>   do_fanotify_mark
>   __x64_sys_fanotify_mark
> 
> The source reproducer ran on ext4 as UID/GID 65534 with CapEff=0 and
> NoNewPrivs=1. It uses ordinary FAN_REPORT_FID groups and races final unlink
> against fd-based mark insertion.

Thanks for the detailed report. Before discussing possible solutions I'd
like to better understand how the race happens because so far that's
unclear to me. You mention you are racing final unlink with fd-based mark
insertion - so I presume we are speaking about inode marks here. Fair.

Now for "final unlink" to reach fsnotify_destroy_marks() it has to go
through:
  d_delete() -> dentry_unlink_inode() -> fsnotify_inoderemove()

But to go through this path, d_delete() has to validate that
dentry->d_lockref.count == 1 which should not be possible if the inode is
still open (and thus the file->f_path holds the dentry reference). What am
I missing here? Which two calls are exactly racing?

FWIW my current suspicion is that calling fanotify_inode_delete() from
fsnotify_inoderemove() is actually too early and we should always leave it
to __destroy_inode() time which would also avoid any races with mark
addition. But the fact that the above race can happen at all makes me
nervous we can be missing some other cornercases...

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR