linux-kernel - Re: [PATCH] mm: cachestat: fix two shmem bugs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKEwX=OH=057RRGS0wv9tMKJK6+w8RMQa8neTPW1wfhpEmuZLw@mail.gmail.com>
Date: Sat, 16 Mar 2024 09:41:04 +0700
From: Nhat Pham <nphamcs@...il.com>
To: Johannes Weiner <hannes@...xchg.org>
Cc: Chengming Zhou <chengming.zhou@...ux.dev>, Andrew Morton <akpm@...ux-foundation.org>, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	Jann Horn <jannh@...gle.com>
Subject: Re: [PATCH] mm: cachestat: fix two shmem bugs

On Fri, Mar 15, 2024 at 4:55 PM Johannes Weiner <hannes@...xchg.org> wrote:
>
> When cachestat on shmem races with swapping and invalidation, there
> are two possible bugs:
>
> 1) A swapin error can have resulted in a poisoned swap entry in the
>    shmem inode's xarray. Calling get_shadow_from_swap_cache() on it
>    will result in an out-of-bounds access to swapper_spaces[].
>
>    Validate the entry with non_swap_entry() before going further.
>
> 2) When we find a valid swap entry in the shmem's inode, the shadow
>    entry in the swapcache might not exist yet: swap IO is still in
>    progress and we're before __remove_mapping; swapin, invalidation,
>    or swapoff have removed the shadow from swapcache after we saw the
>    shmem swap entry.
>
>    This will send a NULL to workingset_test_recent(). The latter
>    purely operates on pointer bits, so it won't crash - node 0, memcg
>    ID 0, eviction timestamp 0, etc. are all valid inputs - but it's a
>    bogus test. In theory that could result in a false "recently
>    evicted" count.
>
>    Such a false positive wouldn't be the end of the world. But for
>    code clarity and (future) robustness, be explicit about this case.
>
>    Bail on get_shadow_from_swap_cache() returning NULL.
>
> Fixes: cf264e1329fb ("cachestat: implement cachestat syscall")
> Cc: stable@...r.kernel.org                              [v6.5+]
> Reported-by: Chengming Zhou <chengming.zhou@...ux.dev>  [Bug #1]
> Reported-by: Jann Horn <jannh@...gle.com>               [Bug #2]
> Signed-off-by: Johannes Weiner <hannes@...xchg.org>

Nice catch! Thanks for the report, Chengming and Jann, and thanks for
the fix, Johannes!
Reviewed-by: Nhat Pham <nphamcs@...il.com>

> ---
>  mm/filemap.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 222adac7c9c5..0aa91bf6c1f7 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -4198,7 +4198,23 @@ static void filemap_cachestat(struct address_space *mapping,
>                                 /* shmem file - in swap cache */
>                                 swp_entry_t swp = radix_to_swp_entry(folio);
>
> +                               /* swapin error results in poisoned entry */
> +                               if (non_swap_entry(swp))
> +                                       goto resched;
> +
> +                               /*
> +                                * Getting a swap entry from the shmem
> +                                * inode means we beat
> +                                * shmem_unuse(). rcu_read_lock()
> +                                * ensures swapoff waits for us before
> +                                * freeing the swapper space. However,
> +                                * we can race with swapping and
> +                                * invalidation, so there might not be
> +                                * a shadow in the swapcache (yet).
> +                                */
>                                 shadow = get_shadow_from_swap_cache(swp);
> +                               if (!shadow)
> +                                       goto resched;
>                         }
>  #endif
>                         if (workingset_test_recent(shadow, true, &workingset))
> --
> 2.44.0
>