On 9/3/26 22:37, Karl Mehltretter wrote:
> Passing an ERR_PTR to kfree() currently reaches virt_to_page() and may
> fault. Warn and return instead, leaving the bad caller visible without
> using the pointer as allocator metadata.
>
> Also reject ERR_PTR values in hardened usercopy. Keep both checks
> separate from ZERO_OR_NULL_PTR(), whose exact matching is required by
> krealloc().
>
> Link:
> https://lore.kernel.org/r/cag48ez05qvn6_gq2tbrra1a_dwqoassyubusu5ymwxx-gqm...@mail.gmail.com
> Link: https://lore.kernel.org/r/202608111716.0FA9DB17@keescook
> Link: https://github.com/KSPP/linux/issues/93
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <[email protected]>
> ---
> mm/slub.c | 3 +++
> mm/usercopy.c | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index f9b56cb439e7..027b44dd7f07 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -6780,6 +6780,9 @@ void kfree(const void *object)
> if (unlikely(ZERO_OR_NULL_PTR(object)))
> return;
>
> + if (WARN_ON(IS_ERR(object)))
Wonder if WARN_ON_ONCE() would be better.
Also wonder about the benefits for hardening (as opposed to debugging).
Without this check it would fault. Now it warns, but hardened setups often
use panic_on_warn anyway, so the result is the same?
> + return;
> +
> page = virt_to_page(object);
> slab = page_slab(page);
> if (!slab) {
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 5de7a518b1b1..c8d8703544c6 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -157,6 +157,9 @@ static inline void check_bogus_address(const unsigned
> long ptr, unsigned long n,
> /* Reject if NULL or ZERO-allocation. */
> if (ZERO_OR_NULL_PTR(ptr))
> usercopy_abort("null address", NULL, to_user, ptr, n);
> +
> + if (IS_ERR_VALUE(ptr))
> + usercopy_abort("ERR_PTR", NULL, to_user, ptr, n);
> }
>
> static inline void check_heap_object(const void *ptr, unsigned long n,
