Re: [RFC PATCH 1/3] luo: Move to feature flags instead of compatibility strings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing payload length validation before accessing feature-dependent fields in `luo_ser`, leading to out-of-bounds reads.
--

commit 8e63def24ae09c6e2674a2bd48e32ef12916dd48
Author: Logan Odell <loganodell@google.com>

luo: Move to feature flags instead of compatibility strings

This commit replaces the compatibility string check with a 128-byte feature
header in the live update serialization structure. It introduces flags for
supported, required, and active features to allow for more flexible structure
evolution.

> diff --git a/kernel/liveupdate/luo_core.c b/kernel/liveupdate/luo_core.c
> --- a/kernel/liveupdate/luo_core.c
> +++ b/kernel/liveupdate/luo_core.c
> @@ -107,26 +107,37 @@ static int __init luo_early_startup(void)
>  		return 0;
>  	}
>  
> -	if (len < sizeof(*luo_ser)) {
> -		pr_err("LUO state is too small (%zu < %zu)\n", len, sizeof(*luo_ser));
> -		return -EINVAL;
> +	luo_ser = phys_to_virt(luo_ser_phys);
> +
> +	if (len < sizeof(struct luo_feature_hdr)) {

[Severity: High]
In luo_early_startup() in kernel/liveupdate/luo_core.c, does this code
sufficiently validate the payload length?

Since len is only checked against the size of struct luo_feature_hdr, it seems
the check for the full struct luo_ser size has been removed.

> +		pr_err("LUO state is too small (%zu < %zu)\n",
> +		       len, sizeof(struct luo_feature_hdr));
> +		err = -EINVAL;
> +		goto out_free_ser;
>  	}

[ ... ]

> +	if (LUO_FEATURE_IS_ACTIVE(luo_ser, LUO_FEATURE_NUMBER)) {
> +		luo_global.liveupdate_num = luo_ser->liveupdate_num;

[Severity: High]
Can this access memory out of bounds?

If the payload length from KHO is large enough to pass the header check but
smaller than the full structure size, this will read luo_ser->liveupdate_num
beyond the validated length.

> +		pr_info("Retrieved live update data, liveupdate number: %lld\n",
> +			luo_global.liveupdate_num);
> +	}
>  
> -	err = luo_session_setup_incoming(luo_ser->sessions_pa);
> -	if (err)
> -		goto out_free_ser;
>  
> -	luo_flb_setup_incoming(luo_ser->flbs_pa);
> +	if (LUO_FEATURE_IS_ACTIVE(luo_ser, LUO_FEATURE_SESSIONS)) {
> +		err = luo_session_setup_incoming(luo_ser->sessions_pa);

[Severity: High]
Can this result in the use of uninitialized or garbage data?

If len does not cover the sessions_pa field, reading it could yield a garbage
physical address that luo_session_setup_incoming() might then dereference,
leading to a panic or state corruption during boot.

> +		if (err)
> +			goto out_free_ser;
> +	}
> +
> +	if (LUO_FEATURE_IS_ACTIVE(luo_ser, LUO_FEATURE_FLBS))
> +		luo_flb_setup_incoming(luo_ser->flbs_pa);

[Severity: High]
Does this code pass a garbage address to luo_flb_setup_incoming()?

Accessing luo_ser->flbs_pa could read beyond the payload if len is smaller
than the full struct luo_ser size.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903023452.721732-1-loganodell@google.com?part=1