Re: [PATCH v3 05/22] x86/boot/slaunch-early: early TXT checks and boot data retrieval

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

On Tue, Jul 08, 2025 at 06:00:13PM +0200, Jan Beulich wrote:
> > +static inline int is_in_pmr(const struct txt_os_sinit_data *os_sinit,
> > +                            uint64_t base, uint32_t size, int check_high)
> > +{
> > +    /* Check for size overflow. */
> > +    if ( base + size < base )
> > +        txt_reset(SLAUNCH_ERROR_INTEGER_OVERFLOW);
> > +
> > +    /* Low range always starts at 0, so its size is also end address. */
> > +    if ( base >= os_sinit->vtd_pmr_lo_base &&
> > +         base + size <= os_sinit->vtd_pmr_lo_size )
>
> If you leverage what the comment says in the 2nd comparsion, why not also
> in the first (which means that could be dropped altogether)? If the start
> is always zero, why does the field exist in the first place?

The range always starts at 0 here because txt_verify_pmr_ranges() reboots
earlier if this assumption doesn't hold.  The field can have non-zero
value, but I guess the more memory is protected the better.  I'll remove
the first part of the check as useless and clarify the comment.

> > +static inline void txt_verify_pmr_ranges(
> > +    const struct txt_os_mle_data *os_mle,
> > +    const struct txt_os_sinit_data *os_sinit,
> > +    const struct slr_entry_intel_info *info,
> > +    uint32_t load_base_addr,
> > +    uint32_t tgt_base_addr,
> > +    uint32_t xen_size)
> > +{
> > +    int check_high_pmr = 0;
>
> Just like Ross mentioned for the return value of is_in_pmr(), this one also
> looks as if it wanted to be bool.

Will update.

> > +    /* All regions accessed by 32b code must be below 4G. */
> > +    if ( os_sinit->vtd_pmr_hi_base + os_sinit->vtd_pmr_hi_size <=
> > +         0x100000000ULL )
> > +        check_high_pmr = 1;
>
> The addition overflowing is only checked later, and that check may be bypassed
> based on the result here. Is that not a possible problem?

Thanks, that looks like a problem to me.  Moved the overflow check from
is_in_pmr() before this check.

> > +    /*
> > +     * If present, check that MBI is covered by PMR. MBI starts with 
> > 'uint32_t
> > +     * total_size'.
> > +     */
> > +    if ( info->boot_params_base != 0 &&
> > +         !is_in_pmr(os_sinit, info->boot_params_base,
> > +                    *(uint32_t *)(uintptr_t)info->boot_params_base,
>
> What is this "MBI" which "starts with 'uint32_t total_size'"? Do you perhaps
> mean multiboot2_fixed_t? If you really can't use a proper structure ref here,
> please at least mention whatever type that is in our code base, so the use
> can be found by e.g. grep.

Yes, it's MultiBoot2 Info.  Nothing precludes using multiboot2_fixed_t,
will update.

> These inline functions are pretty large. Why do they need to be inline ones?
>
> Jan

The functions are run at entry points, one of which is in 32-bit early
code and another in 64-bit EFI.  Having this in the header is simpler
than compiling the code twice.  Despite having many lines, it's just a
sequence of checks, so it didn't seem like too much for a header.

Regards