Re: [PATCH 1/1] tools/libs/light: fix BAR memory address truncation

[Jürgen Groß <jgross@xxxxxxxx>]

On 10.10.25 10:19, Jan Beulich wrote:
> On 10.10.2025 10:00, Jiqian Chen wrote:
> > 64-bit BAR memory address is truncated when removing a passthrough pci
> > device from guest since it uses "unsigned int".
> You talking of address truncation only here, ...
>
> > --- a/tools/libs/light/libxl_pci.c
> > +++ b/tools/libs/light/libxl_pci.c
> > @@ -2001,7 +2001,8 @@ static void pci_remove_detached(libxl__egc *egc,
> >   {
> >       STATE_AO_GC(prs->aodev->ao);
> >       libxl_ctx *ctx = libxl__gc_owner(gc);
> > -    unsigned int start = 0, end = 0, flags = 0, size = 0, irq = 0;
> > +    unsigned long long start = 0, end = 0, flags = 0, size = 0;
> > +    unsigned int irq = 0;
> ... does "flags" really need widening, too?
At least on the system I looked the value was printed as a 64-bit one:

# cat /sys/bus/pci/devices/0000:00:00.0/resource
0x0000000000000000 0x0000000000000000 0x0000000000000000
...

So not widening flags would rely on UB to preserve the evaluated PCI_BAR_IO
flag in case the high 32 bits don't contain 0.

> > @@ -2031,7 +2032,7 @@ static void pci_remove_detached(libxl__egc *egc,
> >       }
> >   
> >       for (i = 0; i < PROC_PCI_NUM_RESOURCES; i++) {
> > -        if (fscanf(f, "0x%x 0x%x 0x%x\n", &start, &end, &flags) != 3)
> > +        if (fscanf(f, "0x%llx 0x%llx 0x%llx\n", &start, &end, &flags) != 3)
> While touching this, don't we want to drop the stray 0x in here? Their
> presence causes bogus input like 0x0x0 to be accepted, afaict.
Hmm, do we really expect a sysfs file to produce bogus output?

Wouldn't it be better to keep the "0x" in order to detect a differing
output format, which could result in silent misbehavior?

I'm not really feeling strong here, as both cases seem highly unlikely.

> > @@ -2040,7 +2041,7 @@ static void pci_remove_detached(libxl__egc *egc,
> >                                                    size, 0);
> >                   if (rc < 0)
> >                       LOGED(ERROR, domid,
> > -                          "xc_domain_ioport_permission error 0x%x/0x%x",
> > +                          "xc_domain_ioport_permission error 0x%llx/0x%llx",
> >                             start,
> >                             size);
> >               } else {
> > @@ -2050,7 +2051,7 @@ static void pci_remove_detached(libxl__egc *egc,
> >                                                   0);
> >                   if (rc < 0)
> >                       LOGED(ERROR, domid,
> > -                          "xc_domain_iomem_permission error 0x%x/0x%x",
> > +                          "xc_domain_iomem_permission error 0x%llx/0x%llx",
> In the hypervisor I would request use of %#llx here; not sure what the
> toolstack's take on this is.
I'd go a little bit further and request to use uint64_t instead of
"unsigned long long" and then use "#"PRIx64 for the format.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature