Re: [PATCH net-next 05/14] calipso: annotate data-races around cache sysctls

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] calipso_cache_add() in net/ipv6/calipso.c still reads
  calipso_cache_bucketsize twice, and the two reads can disagree.

--

calipso: annotate data-races around cache sysctls

This commit adds READ_ONCE() to the lockless readers of
calipso_cache_enabled and calipso_cache_bucketsize in net/ipv6/calipso.c.
The sysctl handlers update these variables with WRITE_ONCE().

> diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
> index e399a42e30839..5e63212b60163 100644
> --- a/net/ipv6/calipso.c
> +++ b/net/ipv6/calipso.c

[ ... ]

> @@ -269,7 +269,7 @@ static int calipso_cache_add(const unsigned char *calipso_ptr,
>  	struct calipso_map_cache_entry *old_entry = NULL;
>  	u32 calipso_ptr_len;
>  
> -	if (!calipso_cache_enabled || calipso_cache_bucketsize <= 0)
> +	if (!READ_ONCE(calipso_cache_enabled) || READ_ONCE(calipso_cache_bucketsize) <= 0)
>  		return 0;
>  
>  	calipso_ptr_len = calipso_ptr[1];
> @@ -289,7 +289,7 @@ static int calipso_cache_add(const unsigned char *calipso_ptr,
>  
>  	bkt = entry->hash & (CALIPSO_CACHE_BUCKETS - 1);
>  	spin_lock_bh(&calipso_cache[bkt].lock);
> -	if (calipso_cache[bkt].size < calipso_cache_bucketsize) {
> +	if (calipso_cache[bkt].size < READ_ONCE(calipso_cache_bucketsize)) {
>  		list_add(&entry->list, &calipso_cache[bkt].list);
>  		calipso_cache[bkt].size += 1;
>  	} else {

[Severity: High]
Can these two separate READ_ONCE() loads of calipso_cache_bucketsize in
calipso_cache_add() return different values?

The first load is unlocked and only guards the early return. The second
load comes later, under the bucket spinlock, after the GFP_ATOMIC
allocations, jhash() and refcount_inc().

The bucket lock does not cover the sysctl. Also, calipso_cache_bucket_size
in net/ipv6/sysctl_net_ipv6.c uses plain proc_dointvec with no
extra1/extra2 bounds, so 0 or a negative value can be written during that
window.

Suppose three things happen together:

  - the first load sees a positive value
  - a concurrent write of 0 lands before the second load
  - the hashed bucket is empty (size == 0)

Then the size check is false, and the eviction branch runs on an empty
list:

    old_entry = list_entry(calipso_cache[bkt].list.prev,
                           struct calipso_map_cache_entry, list);
    list_del(&old_entry->list);
    list_add(&entry->list, &calipso_cache[bkt].list);
    calipso_cache_entry_free(old_entry);

Here old_entry is container_of() applied to the bucket's own list head.
list_del() poisons the head, and list_add() then dereferences
LIST_POISON1.

Wouldn't that oops in softirq context with the bucket lock held?

If execution got past that point, calipso_cache_entry_free() would free
whatever sits at the key and lsm_data offsets of the fake entry. Those
offsets overlap neighbouring buckets. It would then kfree() a pointer into
the middle of the calipso_cache array.

Separately, if only the second load sees a negative value, it is
converted to a huge u32 limit and the bucket size cap is skipped.

This window existed before this patch, since spin_lock_bh() already forced
a reload. However, this commit is meant to fix the racy reads of this
exact variable. Could it take a single snapshot instead? That is what
cipso_v4_cache_add() has done since commit dd44f04b9214ad ("cipso: Fix
data-races around sysctl."):

net/ipv4/cipso_ipv4.c:cipso_v4_cache_add() {
	int bkt_size = READ_ONCE(cipso_v4_cache_bucketsize);
	...
	if (!READ_ONCE(cipso_v4_cache_enabled) || bkt_size <= 0)
		return 0;
	...
	if (cipso_v4_cache[bkt].size < bkt_size) {
	...
}

No later patch in the series changes net/ipv6/calipso.c.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109025425.328445-1-jiayuan.chen%40linux.dev