Re: Brainy: bug in x86/cpu_ucode_intel.c

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Sat, 3 Oct 2015 18:07:38 +0200
    From:        Maxime Villard <max%m00nbsd.net@localhost>
    Message-ID:  <560FFD4A.8010407%m00nbsd.net@localhost>

  | Yes, I understood that. What I don't understand is *why* it allocates
  | memory and wants it to be aligned; 'uh' is not used in the rest of the
  | function.

It looks obvious (based upon looking at an older version, where it
was simply an error if the buffer was not aligned), the

        wrmsr(MSR_BIOS_UPDT_TRIG, (uintptr_t)(sc->sc_blob) + 48);

should be using uh (which is the same value as sc->sc_blob if that
was aligned satisfactorily on entry) rather than sc->sc_blob.

The code as it is is clearly broken, I'm not sure why there's even any
discussion about this.

Just add another variable to remember the kmem_alloc() result so it can be
freed correctly, and fix things to copy the code from the properly aligned
location.

  | Still, this is risky behavior.

It isn't just risky, it is broken.   If kmem_alloc() was to
guarantee 16 byte alignment, which it apparently doesn't, then
all the rouning stuff would be unnecessary and useless, and should
just be removed.   If it doesn't guarantee that, then what is being
freed might not be the same as what was allocated, which is broken.

If it happens that (despite lack of the guarantee) kmem_alloc() happens
to be returning 16 byte aligned memory, so the roundup2() does nothing,
and the kmem_free() works, that's just a fluke, and relying upon it is
absurd.

Cease the silly discussion, and just fix it!

kre