Avoid using BOOTMOD_XEN region for other purposes or boot modules
which could result in memory corruption or undefined behaviour.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/setup.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 6d156c3a40..39375b3366 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
 #include <xen/bug.h>
+#include <xen/bootfdt.h>
 #include <xen/compile.h>
 #include <xen/device_tree.h>
 #include <xen/init.h>
@@ -26,6 +27,8 @@ unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
 void __init noreturn start_xen(unsigned long bootcpu_id,
                                paddr_t dtb_addr)
 {
+    struct bootmodule *xen_bootmodule;
+
     remove_identity_mapping();
 
     set_processor_id(0);
@@ -44,6 +47,13 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
               "Please check your bootloader.\n",
               dtb_addr, BOOT_FDT_VIRT_SIZE);
 
+    /* Register Xen's load address as a boot module. */
+    xen_bootmodule = add_boot_module(BOOTMOD_XEN,
+                                     virt_to_maddr(_start),
+                                     (paddr_t)(_end - _start), false);
+
+    BUG_ON(!xen_bootmodule);
+
     printk("All set up\n");
 
     machine_halt();
-- 
2.46.1

On 30.09.2024 17:08, Oleksii Kurochko wrote:
> @@ -26,6 +27,8 @@ unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>  void __init noreturn start_xen(unsigned long bootcpu_id,
>                                 paddr_t dtb_addr)
>  {
> +    struct bootmodule *xen_bootmodule;

With just the uses below this can be pointer-to-const. Question of course
is whether you already know of further uses.

> @@ -44,6 +47,13 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>                "Please check your bootloader.\n",
>                dtb_addr, BOOT_FDT_VIRT_SIZE);
>  
> +    /* Register Xen's load address as a boot module. */
> +    xen_bootmodule = add_boot_module(BOOTMOD_XEN,
> +                                     virt_to_maddr(_start),
> +                                     (paddr_t)(_end - _start), false);

There's no real need for the cast, is there? Plus if anything, it would be
more to size_t than to paddr_t.

Jan

On Tue, 2024-10-01 at 17:49 +0200, Jan Beulich wrote:
> On 30.09.2024 17:08, Oleksii Kurochko wrote:
> > @@ -26,6 +27,8 @@ unsigned char __initdata
> > cpu0_boot_stack[STACK_SIZE]
> >  void __init noreturn start_xen(unsigned long bootcpu_id,
> >                                 paddr_t dtb_addr)
> >  {
> > +    struct bootmodule *xen_bootmodule;
> 
> With just the uses below this can be pointer-to-const. Question of
> course
> is whether you already know of further uses.
It could be dropped as it is used only for BUG_ON(!xen_bootmodule) as
it looks to me a little bit better then:
    BUG_ON(!add_boot_module(BOOTMOD_XEN, virt_to_maddr(_start),
                            (_end - _start), false));
But I am okay to drop xen_bootmodule variable.

> 
> > @@ -44,6 +47,13 @@ void __init noreturn start_xen(unsigned long
> > bootcpu_id,
> >                "Please check your bootloader.\n",
> >                dtb_addr, BOOT_FDT_VIRT_SIZE);
> >  
> > +    /* Register Xen's load address as a boot module. */
> > +    xen_bootmodule = add_boot_module(BOOTMOD_XEN,
> > +                                     virt_to_maddr(_start),
> > +                                     (paddr_t)(_end - _start),
> > false);
> 
> There's no real need for the cast, is there? Plus if anything, it
> would be
> more to size_t than to paddr_t.
In this case the cast isn't really needed. I will drop it.

Thanks.

~ Oleksii

On 02.10.2024 13:25, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-10-01 at 17:49 +0200, Jan Beulich wrote:
>> On 30.09.2024 17:08, Oleksii Kurochko wrote:
>>> @@ -26,6 +27,8 @@ unsigned char __initdata
>>> cpu0_boot_stack[STACK_SIZE]
>>>  void __init noreturn start_xen(unsigned long bootcpu_id,
>>>                                 paddr_t dtb_addr)
>>>  {
>>> +    struct bootmodule *xen_bootmodule;
>>
>> With just the uses below this can be pointer-to-const. Question of
>> course
>> is whether you already know of further uses.
> It could be dropped as it is used only for BUG_ON(!xen_bootmodule) as
> it looks to me a little bit better then:
>     BUG_ON(!add_boot_module(BOOTMOD_XEN, virt_to_maddr(_start),
>                             (_end - _start), false));

Yet that's undesirable for other reasons. Did you consider

    if ( !add_boot_module(BOOTMOD_XEN, virt_to_maddr(_start),
                          _end - _start, false) )
        BUG();

? Maybe panic() would be even better for cases like this one.

Jan

On Wed, 2024-10-02 at 14:11 +0200, Jan Beulich wrote:
> On 02.10.2024 13:25, oleksii.kurochko@gmail.com wrote:
> > On Tue, 2024-10-01 at 17:49 +0200, Jan Beulich wrote:
> > > On 30.09.2024 17:08, Oleksii Kurochko wrote:
> > > > @@ -26,6 +27,8 @@ unsigned char __initdata
> > > > cpu0_boot_stack[STACK_SIZE]
> > > >  void __init noreturn start_xen(unsigned long bootcpu_id,
> > > >                                 paddr_t dtb_addr)
> > > >  {
> > > > +    struct bootmodule *xen_bootmodule;
> > > 
> > > With just the uses below this can be pointer-to-const. Question
> > > of
> > > course
> > > is whether you already know of further uses.
> > It could be dropped as it is used only for BUG_ON(!xen_bootmodule)
> > as
> > it looks to me a little bit better then:
> >     BUG_ON(!add_boot_module(BOOTMOD_XEN, virt_to_maddr(_start),
> >                             (_end - _start), false));
> 
> Yet that's undesirable for other reasons. Did you consider
> 
>     if ( !add_boot_module(BOOTMOD_XEN, virt_to_maddr(_start),
>                           _end - _start, false) )
>         BUG();
> 
> ? Maybe panic() would be even better for cases like this one.
It looks fine to me. Thanks for suggestion.

~ Oleksii