On Mon, Jun 05, 2023 at 05:43:33PM +0200, Borislav Petkov wrote:
> On Thu, Jun 01, 2023 at 09:25:39PM +0300, Kirill A. Shutemov wrote:
> > +void accept_memory(phys_addr_t start, phys_addr_t end)
> > +{
> > + struct efi_unaccepted_memory *unaccepted;
> > + unsigned long range_start, range_end;
> > + unsigned long flags;
> > + u64 unit_size;
> > +
> > + if (efi.unaccepted == EFI_INVALID_TABLE_ADDR)
> > + return;
>
> efi_get_unaccepted_table() already does this test.
Okay.
> > + unaccepted = efi_get_unaccepted_table();
> > + if (!unaccepted)
> > + return;
>
> So this looks weird: callers can call accept_memory() and that function
> can fail. But they can't know whether it failed or not because it
> returns void.
It is not a failure here. If there's no unaccepted memory in the system
accept_memory() always succeeds.
> > + unit_size = unaccepted->unit_size;
> > +
> > + /*
> > + * Only care for the part of the range that is represented
> > + * in the bitmap.
> > + */
> > + if (start < unaccepted->phys_base)
> > + start = unaccepted->phys_base;
>
> So this silently trims start...
>
> > + if (end < unaccepted->phys_base)
> > + return;
>
> But fails only when end is outside of range.
>
> I'd warn here at least. And return an error so that the callers know.
There's nothing to warn about. The range (or part of it) is not
represented in the bitmap because it is not unaccepted. We only allocate
bitmap for the range that has unaccepted memory. It can reduce memory
overhead on the bitmap if the unaccepted memory starts very high or ends
early, but there's something else very high in physical addresss space.
> > + /* Translate to offsets from the beginning of the bitmap */
> > + start -= unaccepted->phys_base;
> > + end -= unaccepted->phys_base;
> > +
> > + /* Make sure not to overrun the bitmap */
> > + if (end > unaccepted->size * unit_size * BITS_PER_BYTE)
> > + end = unaccepted->size * unit_size * BITS_PER_BYTE;
>
> How is all that trimming not important to the caller?
>
> It would assume that its memory got accepted but not really.
See above: not represented in the bitmap means pre-accepted.
...
> > + spin_lock_irqsave(&unaccepted_memory_lock, flags);
> > + while (start < end) {
> > + if (test_bit(start / unit_size, unaccepted->bitmap)) {
> > + ret = true;
> > + break;
>
> I have a faint memory we've had this before but you need to check
> *every* bit in the unaccepted bitmap before returning true. Doh.
Yes, it was discussed before. Here's context:
https://lore.kernel.org/all/Ynt8vDY78/YeXO99@xxxxxxx
--
Kiryl Shutsemau / Kirill A. Shutemov
