Re: [PATCH v2 1/2] board: sifive: unmatched: use zero copy for initrd

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3810 bytes --]

On Tue, Nov 09, 2021 at 05:27:52PM +0100, Heinrich Schuchardt wrote:
> On 11/9/21 17:07, Tom Rini wrote:
> > On Tue, Nov 09, 2021 at 04:50:27PM +0100, Heinrich Schuchardt wrote:
> > > On 11/9/21 16:46, Tom Rini wrote:
> > > > On Tue, Nov 09, 2021 at 03:46:00PM +0100, Heinrich Schuchardt wrote:
> > > > 
> > > > > Booting Ubuntu Impish showed the following output:
> > > > > 
> > > > >       relocaddr   = 0x00000000fff60000
> > > > > 
> > > > >       Loading Ramdisk to fa118000, end fffff19d ...
> > > > > 
> > > > > The initrd is overwriting the U-Boot binary. Booting fails.
> > > > > 
> > > > > There is no need to copy the initrd from $ramdisk_addr_r.
> > > > > Set init_high = ~0UL to use zero copy. Do the same for the device tree.
> > > > > 
> > > > > Same for the devicetree.
> > > > > 
> > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > ---
> > > > > v2:
> > > > > 	Don't copy fdt either.
> > > > > ---
> > > > >    include/configs/sifive-unmatched.h | 2 ++
> > > > >    1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/include/configs/sifive-unmatched.h b/include/configs/sifive-unmatched.h
> > > > > index f68d7d7676..69a4eb2f2a 100644
> > > > > --- a/include/configs/sifive-unmatched.h
> > > > > +++ b/include/configs/sifive-unmatched.h
> > > > > @@ -59,6 +59,8 @@
> > > > >    	"name=system,size=-,bootable,type=${type_guid_gpt_system};"
> > > > >    #define CONFIG_EXTRA_ENV_SETTINGS \
> > > > > +	"fdt_high=0xffffffffffffffff\0" \
> > > > > +	"initrd_high=0xffffffffffffffff\0" \
> > > > >    	"kernel_addr_r=0x84000000\0" \
> > > > >    	"fdt_addr_r=0x88000000\0" \
> > > > >    	"scriptaddr=0x88100000\0" \
> > > > 
> > > > I know I'm doing this out of order, but I'm going to repeat myself here
> > > > too.  You cannot disable device tree relocation.  I cannot count the
> > > > number of hours that have been wasted because of this mis-feature due to
> > > > misalignment of the device tree or overwriting of the device tree and
> > > > then U-Boot not fixing it because it was told not to, and then people
> > > > and projects wasting countless hours over it.  It's why checkpatch.pl
> > > > throws out an ERROR on this now.  I didn't yell even more loudly
> > > > previously at riscv because as it was missing the arch_lmb portion to
> > > > avoid overwriting U-Boot at run-time, it still was a problem.  But
> > > > that's been fixed.  So, no.  NAK.
> > > 
> > > Why should the devicetree relocated?
> > > This should never have been enabled on RISC-V.
> > 
> > To repeat myself, because RISC-V has been broken until very recently and
> > lacked the parts of lmb to avoid overwriting U-Boot while running, is
> > why any platforms have been allowed in with fdt/initrd_high set to
> > disable relocation.  As that problem has now been fixed, fdt relocation
> > must be re-enabled on the currently wrong platforms, and will not be
> > allowed on new platforms.
> > 
> > There are specific deployment cases where the developer can choose to
> > disable relocation because they know that there will never be a way for
> > things to be done in an overlapping manner because the system is locked
> > down.  That is very rarely the case for mainline and absolutely not the
> > case for a general purpose board like the unmatched.
> 
> __lmb_alloc_base() seems not be integrated with the UEFI sub-system. So UEFI
> might hand out memory marked as reserved in the LMB sub-system.
> 
> I guess this is still a topic to be addressed.

If UEFI can still end up getting U-Boot overwritten, yes, that needs to
be addressed.  Only slightly surprised one of the capture-the-flag or
similar events hasn't come to us yet with some CVEs related to that,
too.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]