Re: [PATCH 1/3] xen/livepatch: simplify and unify logic in prepare_payload()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.09.2024 11:43, Andrew Cooper wrote:
> On 23/09/2024 8:46 am, Roger Pau Monné wrote:
>> On Sun, Sep 22, 2024 at 11:19:01AM +0200, Andrew Cooper wrote:
>>> On 20/09/2024 11:36 am, Roger Pau Monne wrote:
>>>> +{
>>>> +    const Elf_Note *n = sec->load_addr;
>>>> +    int rc;
>>>> +
>>>> +    ASSERT(sec);
>>> This needs to turn back into a runtime check.  Now, if a livepatch is
>>> missing one of the sections, we'll dereference NULL below, rather than
>>> leaving no data in the struct livepatch_build_id.
>> Loading should never get here without those sections being present,
>> check_special_sections() called earlier will return error if any of
>> the sections is not present, hence the ASSERT() is fine IMO.
> 
> Ah, in which case, can we please have:
> 
> /* Existence of note sections already confirmed in
> check_special_sections() */
> ASSERT(sec);
> 
> Just to show that someone did think about the provenance of the pointer,
> and where to look to check.

Yet then sec was de-referenced already ahead of the assertion, which
static checkers may have to say something about.

Jan