Re: [PATCH bpf-next 1/5] bpf: add bpf_memcmp kfunc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] bpf_memcmp uses signed `char` for memory comparison, violating standard memcmp behavior and causing incorrect sorting for bytes >= 0x80.
- [High] Unbounded O(N^2) loop in bpf_strpbrk and bpf_strcspn can cause a kernel soft lockup.
- [High] Arbitrary kernel memory read oracle via unverified `__ign` arguments in bpf_memcmp.
- [Medium] bpf_memcmp returns -1 to indicate `ptr1 < ptr2`, which collides with the standard error code -EPERM.
--

commit 0b32f895c504b5a659411b32505ad2719b89eec0
Author: Longjun Tang <tanglongjun@kylinos.cn>

bpf: add bpf_memcmp kfunc
    
Add bpf_memcmp() for binary memory comparison. Unlike bpf_strcmp,
null bytes do not terminate the comparison. Returns 0, -1, or 1.
Registered in common_btf_ids.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 8e196c9b7c50f..e444c58cad9e9 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -4070,6 +4070,48 @@ __bpf_kfunc int bpf_strcspn(const char *s__ign, const char *reject__ign)

[Severity: High]
Can the nested loops in bpf_strcspn() and bpf_strpbrk() cause a kernel soft
lockup?

If a BPF program passes two distinct 65,535-character strings that do not
share any characters, the inner loop executes 65,535 times and cleanly
breaks at the null byte. Because the inner loop breaks early, the outer
loop continues, resulting in up to 4.29 billion calls to
__get_kernel_nofault(). Can this stall the CPU and trigger a watchdog
timeout?

>  	return -EFAULT;
>  }
>  
> +/**
> + * bpf_memcmp - Compare two memory regions byte by byte
[ ... ]
> + */
> +__bpf_kfunc int bpf_memcmp(const void *ptr1__ign, const void *ptr2__ign,
> +			   size_t size)

[Severity: High]
Does using the __ign suffix for these pointers bypass the BPF verifier's
pointer safety checks?

Since bpf_memcmp() takes an explicit size parameter, using __ign instead
of standard bounds-checked arguments might allow a program to pass an
unverified scalar as a kernel address, potentially creating an oracle to
read arbitrary kernel memory.

> +{
> +	const char *p1 = ptr1__ign, *p2 = ptr2__ign;
> +	char c1, c2;
> +	size_t i;
> +
> +	if (size > XATTR_SIZE_MAX)
> +		return -E2BIG;
> +	if (!copy_from_kernel_nofault_allowed(p1, size) ||
> +	    !copy_from_kernel_nofault_allowed(p2, size))
> +		return -ERANGE;
> +
> +	guard(pagefault)();
> +	for (i = 0; i < size; i++) {
> +		__get_kernel_nofault(&c1, p1 + i, char, err_out);
> +		__get_kernel_nofault(&c2, p2 + i, char, err_out);
> +		if (c1 != c2)
> +			return c1 < c2 ? -1 : 1;

[Severity: High]
Will using signed char variables for the memory comparison result in
incorrect sorting for bytes with values of 0x80 or higher?

On architectures where char is signed, a byte with the most significant bit
set (like 0xFF) evaluates as a negative number. When compared against 0x00,
-1 < 0 evaluates to true, causing the function to incorrectly return -1.

[Severity: Medium]
Does returning -1 for a valid comparison result collide with the standard
-EPERM error code?

Since this function also returns standard negative error codes (like
-EFAULT or -E2BIG), standard BPF error checking patterns like if (ret < 0)
might mistakenly interpret a valid ptr1 < ptr2 result as an operation
failure.

> +	}
> +	return 0;
> +err_out:
> +	return -EFAULT;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623030803.44798-1-lange_tang@163.com?part=1