Re: install/49470

[gnats-admin%netbsd.org@localhost]

The following reply was made to PR port-amd64/49470; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: Andrius V <vezhlys%gmail.com@localhost>
Cc: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost
Subject: Re: install/49470
Date: Thu, 15 Aug 2024 23:10:35 +0000

 > Date: Fri, 16 Aug 2024 00:50:03 +0300
 > From: Andrius V <vezhlys%gmail.com@localhost>
 >=20
 > After many additional debugging sessions and big help from Taylor I
 > narrowed down the issue to the bi_getmemmap() call, or more
 > specifically to getmementry(&i, buf)
 > (https://nxr.netbsd.org/xref/src/sys/arch/i386/stand/lib/biosmemx.S#87)
 > This method invokes BIOS call and fills the buffer[5] with defined
 > values. For some reason though, with ACPI 3.0 it actually seems to get
 > 6 values,
 > thus leading stack corruption (overflow(?)). The sixth value is always
 > 1, and is set after each and every call.
 > All other buf values [0-4] and number of iterations match completely
 > the ones when ACPI 2.0 is set.
 
 Amazing!  Thanks for doing the work to dig into this!
 
 With that clue, I poked around a little bit and found this fragment on
 wiki.osdev.org:
 
   `The format of an entry is 2 uint64_t's and a uint32_t in the 20
    byte version, plus one additional uint32_t in the 24 byte ACPI 3.0
    version.'
 
    https://wiki.osdev.org/Detecting_Memory_(x86)#BIOS_Function:_INT_0x15,_E=
 AX_=3D_0xE820
 
 I looked at the page history to see where this came from, and found
 that from 2008 when the page was first written until 2023(!), the text
 read:
 
   `The format of an entry is 2 uint64_t's and a uint32_t in the 20
    byte version, plus one additional uint32_t in the 24 byte ACPI 3.0
    version (but nobody has ever seen a 24 byte one).'
 
    https://wiki.osdev.org/index.php?title=3DDetecting_Memory_(x86)&oldid=3D=
 27401#BIOS_Function:_INT_0x15,_EAX_=3D_0xE820
 
 The edit comment says: `I have found an bios that use 24 byte entrie
 for int 0x15 function 0xe820'
 
 This page was a rewrite of an earlier page, whose last edition says:
 
    ACPI 3.0 notes
    ...
    * A 32 bit set of flags ("Extended Attibutes") has been appended to
      the end of the 20 byte structure, making it 24 bytes.
 
    https://wiki.osdev.org/index.php?title=3DHow_Do_I_Determine_The_Amount_O=
 f_RAM&oldid=3D4759#Counting_RAM_using_the_BIOS
 
 I looked up INT 15h E820h in the ACPI 3.0 spec, and it says:
 
    Table 14-2  Input to the INT 15h E820h Call
    ...
    ES:DI        Buffer Pointer  Pointer to an Address Range Descriptor
                                 structure that the BIOS fills in.
    ECX          Buffer Size     The length in bytes of the structure
                                 passed to the BIOS.  The BIOS fills in
                                 the number of bytes of the structure
                                 indicated in the ECX register,
                                 maximum, or whatever amount of the
                                 structure the BIOS implements.  The
                                 minimum size that must be supported by
                                 both the BIOS and the caller is 20
                                 bytes.  Future implementations might
                                 extend this structure.
 
    ...
 
    Table 14-4  Address Range Descriptor Structure
    Offset       Name                Description
    ...
    16           Type                Address type of this range
    20           Extended Attributes See Table 14-5
 
    https://uefi.org/sites/default/files/resources/ACPI_3.pdf#page=3D404
 
 So this ACPI 3.0 system is probably just ignoring the size we passed
 in ecx (which is definitely 20, not 24!), which is rude, but there's
 probably not much we can do to convince the firmware vendor to ship
 updates to these decade-old laptops.
 
 > Workaround can be allocating more memory to buf, for example setting
 > buf size to 6 (buf[6]):
 > https://nxr.netbsd.org/xref/src/sys/arch/i386/stand/lib/bootinfo_memmap.c=
 #40
 
 So yes, I think that is the correct thing to do here.
 
 Now, the BIOS is obviously violating the rules here -- we passed in a
 buffer size of 20 bytes, and it wrote out 24 bytes.  In principle if
 we wanted to hear about the extended attributes, we could also change
 getmementry to set ecx=3D24, but that's a much riskier change.
 
 (It might even be reasonable to make a struct out of this -- not
 really sure why we use an array on the stack -- but refactoring this
 isn't as important as avoiding the stack buffer overrun!)
 
 So I suggest we just change `int buf[5]' to `int buf[6]' in:
 - bi_getmemmap in bootinfo_memmap.c
 - getextmemx in getextmemx.c
 - the comment over getmementry in biosmemx.S
 (and make sure there's no sizeof(buf) that changes).
 
 We should also put a reference to all this bug, the relevant part of
 the ACPI 3.0 spec, and the wiki.osdev.org page in comments so the next
 person to stumble upon buf[6] doesn't change it back to buf[5] or
 wonder what that extra word is doing!