Re: [PATCH v9 6/6] x86/boot: add cmdline to struct boot_domain

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-11-20 12:57, Daniel P. Smith wrote:
> On 11/15/24 13:20, Jason Andryuk wrote:
> > On 2024-11-15 08:12, Daniel P. Smith wrote:>>
> > Just xmalloc_array since it'll be overwritten immediately?
> Yes and no, the concern I was worried about is that the allocation may 
> end up being slight bigger than what is copied in to the buffer. If we 
> do not zero the buffer, then those trailing bytes will have random data 
> in them. In a perfect world, nothing should ever reach those bytes based 
> on the current usage of the buffer. But from my perspective, it would be 
> safer to zero the buffer than rely on the world being perfect. I am not 
> fixed on not switching, just providing my 2 cents,
I only looked at the PVH case, but strlen() is used there to obtain the 
copy length.  I think it's unnecessary and the code doesn't require a 
zeroed buffer.  But I also realize it's safer to start from zeroed.
> > > +            panic("Error allocating cmdline buffer for %pd\n", d);
> > > +
> > >           if ( bd->kernel->cmdline_pa )
> > > -            safe_strcpy(cmdline,
> > > -                        cmdline_cook(__va(bd->kernel->cmdline_pa), 
> > > bi->loader));
> > > +            strlcpy(cmdline,
> > > +                    cmdline_cook(__va(bd->kernel->cmdline_pa),bi- 
> > > >loader),
Also I just noticed a missing space: "cmdline_pa),bi"

> > > +                    cmdline_size);
> > >           if ( bi->kextra )
> > >               /* kextra always includes exactly one leading space. */
> > > -            safe_strcat(cmdline, bi->kextra);
> > > +            strlcat(cmdline, bi->kextra, cmdline_size);
> > >           /* Append any extra parameters. */
> > >           if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> > > -            safe_strcat(cmdline, " noapic");
> > > +            strlcat(cmdline, " noapic", cmdline_size);
> > >           if ( (strlen(acpi_param) == 0) && acpi_disabled )
> > >           {
> > > @@ -1028,17 +1055,21 @@ static struct domain *__init 
> > > create_dom0(struct boot_info *bi)
> > >           if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
> > >           {
> > > -            safe_strcat(cmdline, " acpi=");
> > > -            safe_strcat(cmdline, acpi_param);
> > > +            strlcat(cmdline, " acpi=", cmdline_size);
> > > +            strlcat(cmdline, acpi_param, cmdline_size);
> > >           }
> > > -        bd->kernel->cmdline_pa = __pa(cmdline);
> > > +        bd->cmdline = cmdline;
> > > +        bd->kernel->cmdline_pa = __pa(bd->cmdline);
> > Should cmdline_pa go away if we now have a valid cmdline variable?
> In the PVH dom0 case, we are still relying on cmdline_pa as the 
> reference to get at the command line in the function pvh_load_kernel(). 
> With the introduction of cmdline to boot_domain, I could convert the 
> interface of pvh_load_kernel() to take the boot_domain instance, 
> removing the need to update cmdline_pa. Not sure if you were asking 
> this, but as for cmdline_pa going completely away, that is not possible. 
> First the sequence of events do not allow it, and there is an one-off 
> case for PVH dom0 where the cmdline_pa of the initrd module is copied 
> into the domain.
I was thinking from this point forward only boot_domain->cmdline is 
necessary.  Maybe even zero-ing cmdline_pa?  With a valid pointer in 
cmdline, cmdline_pa shouldn't be necessary anymore.
Regards,
Jason