[PATCH 1/1] sandbox: ensure that state->ram_buf is in low memory

[Simon Glass]

Hi Heinrich,

On Fri, 14 May 2021 at 18:34, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Am 14. Mai 2021 22:44:32 MESZ schrieb Simon Glass <sjg at chromium.org>:
> >Hi Heinrich,
> >
> >On Fri, 14 May 2021 at 00:11, Heinrich Schuchardt <xypron.glpk at gmx.de>
> >wrote:
> >>
> >> On 5/14/21 1:56 AM, Simon Glass wrote:
> >> > Hi Heinrich,
> >> >
> >> > On Thu, 13 May 2021 at 08:53, Heinrich Schuchardt
> ><xypron.glpk at gmx.de> wrote:
> >> >>
> >> >> On 5/13/21 4:36 PM, Simon Glass wrote:
> >> >>> Hi Heinrich,
> >> >>>
> >> >>> On Wed, 12 May 2021 at 15:28, Heinrich Schuchardt
> ><xypron.glpk at gmx.de> wrote:
> >> >>>>
> >> >>>> Am 12. Mai 2021 18:01:17 MESZ schrieb Simon Glass
> ><sjg at chromium.org>:
> >> >>>>> Hi Heinrich,
> >> >>>>>
> >> >>>>> On Tue, 11 May 2021 at 13:03, Heinrich Schuchardt
> ><xypron.glpk at gmx.de>
> >> >>>>> wrote:
> >> >>>>>>
> >> >>>>>> Addresses in state->ram_buf must be in the low 4 GiB of the
> >address
> >> >>>>> space.
> >> >>>>>> Otherwise we cannot correctly fill SMBIOS tables. This shows
> >up in
> >> >>>>> warnings
> >> >>>>>> like:
> >> >>>>>>
> >> >>>>>>       WARNING: SMBIOS table_address overflow 7f752735e020
> >> >>>>>
> >> >>>>> This sounds like a bug in the smbios-table code. For sandbox it
> >should
> >> >>>>> perhaps use addresses instead of pointers.
> >> >>>>>
> >> >>>>> I think that code (that I unfortunately wrote) was an
> >expeditious way
> >> >>>>> of getting it running, but is not correct.
> >> >>>>
> >> >>>> The field you are filling is only 32bit wide. I wonder how that
> >table is meant to work on systems where the lowest memory address is
> >above 4 GiB. Such ARMv8 systems exist.
> >> >>>
> >> >>> map_to_sysmem() will give you a 32-bit wide address. Yes SMBIOS
> >is
> >> >>> legacy and designed for 4GB.
> >> >>
> >> >> I know map_to_sysmem(). But you wrote in lib/smbios.c:487:
> >> >>
> >> >> /*
> >> >>    * We must use a pointer here so things work correctly on
> >sandbox. The
> >> >>    * user of this table is not aware of the mapping of addresses
> >to
> >> >>    * sandbox's DRAM buffer.
> >> >>    */
> >> >>
> >> >> For testing you could start a binary with command 'bootefi' or
> >'booti'
> >> >> and that binary would analyze the table. So yes, your comment
> >holds true
> >> >> and you must not use map_to_sysmem() here.
> >> >
> >> > Yes, that is a good point. Perhaps I was not mad when I wrote that.
> >> > What is using the tables on sandbox?
> >>
> >> The UEFI shell has a command smbiosview to display the SMBIOS table.
> >>
> >> With the current U-Boot sandbox it crashes. I do not know what the
> >cause is:
> >>
> >> FS0:\> smbiosview
> >>
> >> Segmentation violation
> >> pc = 0x7fb0df88d17e, pc_reloc = 0x7fb0cf88d17e
> >>
> >> UEFI image [0x00007fb0df836000:0x00007fb0df920c5f] pc=0x5717e
> >> '/Shell_x64.efi'
> >>
> >> Here is some of the output for my laptop:
> >>
> >> SMBIOS Entry Point Structure:
> >> Anchor String:        _SM_
> >> EPS Checksum:         0xEE
> >> Entry Point Len:      31
> >> Version:              3.1
> >> Number of Structures: 62
> >> Max Struct size:      145
> >> Table Address:        0x986E9000
> >> Table Length:         2316
> >> Entry Point revision: 0x0
> >> SMBIOS BCD Revision:  0x31
> >> Inter Anchor:         _DMI_
> >> Inter Checksum:       0x4E
> >> Formatted Area:
> >>    00000000: 00 00 00 00 00                                   *.....*
> >>
> >> =========================================================
> >> Query Structure, conditions are:
> >> QueryType   = Random
> >> QueryHandle = Random
> >> ShowType    = SHOW_DETAIL
> >>
> >>
> >> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
> >> $
> >> =========================================================
> >> Type=18, Handle=0x0
> >> Dump Structure as:
> >> Index=0,Length=0x19,Addr=0x986E9019
> >> 00000000: 12 17 00 00 03 02 02 00-00 00 00 00 00 00 80 00
> >> *................*
> >> 00000010: 00 00 80 00 00 00 80 00-00
> >*.........*
> >>
> >> Enter to continue, :q to exit, :[0-3] to change mode, /? for help
> >> $Structure Type: 32-bit Memory Error Information
> >> Format part Len : 23
> >> Structure Handle: 0
> >> 32-bit Memory Error Information - Type:   OK
> >> Memory Error - Error granularity:   Unknown
> >> Memory Error - Error Operation:   Unknown
> >> VendorSyndrome: 0x0
> >> MemoryArrayErrorAddress: 0x80000000
> >> DeviceErrorAddress: 0x80000000
> >> ErrorResolution: 0x80000000
> >
> >OK, I am not sure what to make of that except that it is every bit as
> >impenetrable as I would expect from UEFI.
>
> This is a dump of the SMBIOS tables. Why do you refer to UEFI?

How do you fit so many questions in one email? :-)

I assumed smbiosview is a UEFI command because of the prompt. I have
never heard of the command.

>
> >
> >Is this on a Risc-V host? I am not sure why the first alloc would get
> >the requested address and not the second. Is it because we specify
> >0x10000000 as the requested address? Is that what you actually get in
> >this case?
>
> The observation is reproducible on x86_64.
>
> Two mmap calls cannot receive the same address without calling munmap in between.
>
> The first call *might* return the address 0x10000000 that you ask for.
>
> If the first call cannot receive it because it is not available, why should subsequent calls succeed?

The goal is to keep the addresses small since 64-bit addresses are a
pain when debugging. I had hoped that mmap() would try to find an
address near the one requested, for subsequent calls, but perhaps that
hope is forlorn. It certainly doesn't seem to do that on x86.

>
> >
> >If we drop that value on subsequent calls to mmap(), does it fix the
> >problem?
>
> If you drop that value you get a random address. What would be better in this case?

I just tried it on x86 and we get a 'random' address anyway, whatever
is passed, once it has used the requested address. I think the address
hint only works if it works. If not, then linux starts allocating from
another address. Perhaps this linux behaviour has changed in recent
years.

>
> >
> >I think we should update the commit message to say we are hoping to
> >get 0x10000000 for the RAM buffer (although of course there is a 4K
> >region at the start of it) and that xx arch provides large addresses
> >for subsequent mmap()s.
>
> What 4 KiB region are you referring to?

If you look at os_malloc() you will see that it adds a 4KB header
before each block it allocates.

>
> Why do you expect any specific address in subsequent calls? The man-page states "If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint."
>
> For performance reasons the kernel should not care about the hint in this case.

OK.

>
> >
> >Also, how about upgrading the smbios warning to an error. I think that
> >is what it is and it should stop execution.
>
> For errors we use log_err(). But where is the error? mmap() comes with absolutely no guarantee that it will assign the address that you are requesting or anything close to it.
>
> Smbios is not important enough to make the sandbox unusable in this case.

Well if the tables are invalid we should not pass them on...can we at
least do that?

Regards,
Simon
[..]