libxl__xs_directory() can potentially return NULL without setting `n`.
As `n` isn't initialised, we need to check libxl__xs_directory()
return value before checking `n`. Otherwise, `n` might be non-zero
with `bdfs` NULL which would lead to a segv.

Reported-by: "G.R." <firemeteor@users.sourceforge.net>
Fixes: 57bff091f4 ("libxl: add 'name' field to 'libxl_device_pci' in the IDL...")
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Hi G.R., you've reported a segv in name2bdf(), and that the only
potential segv I've found. I hope it's the same one as you've
experienced!
---
 tools/libs/light/libxl_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 96f88795b6..f4c4f17545 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -859,7 +859,7 @@ static int name2bdf(libxl__gc *gc, libxl_device_pci *pci)
     int rc = ERROR_NOTFOUND;
 
     bdfs = libxl__xs_directory(gc, XBT_NULL, PCI_INFO_PATH, &n);
-    if (!n)
+    if (!bdfs || !n)
         goto out;
 
     for (i = 0; i < n; i++) {
-- 
Anthony PERARD

On 11.07.22 12:38, Anthony PERARD wrote:
> libxl__xs_directory() can potentially return NULL without setting `n`.
> As `n` isn't initialised, we need to check libxl__xs_directory()
> return value before checking `n`. Otherwise, `n` might be non-zero
> with `bdfs` NULL which would lead to a segv.
> 
> Reported-by: "G.R." <firemeteor@users.sourceforge.net>
> Fixes: 57bff091f4 ("libxl: add 'name' field to 'libxl_device_pci' in the IDL...")
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

On Mon, Jul 11, 2022 at 7:03 PM Juergen Gross <jgross@suse.com> wrote:
>
> On 11.07.22 12:38, Anthony PERARD wrote:
> > libxl__xs_directory() can potentially return NULL without setting `n`.
> > As `n` isn't initialised, we need to check libxl__xs_directory()
> > return value before checking `n`. Otherwise, `n` might be non-zero
> > with `bdfs` NULL which would lead to a segv.
> >
> > Reported-by: "G.R." <firemeteor@users.sourceforge.net>
> > Fixes: 57bff091f4 ("libxl: add 'name' field to 'libxl_device_pci' in the IDL...")
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Hi Anthony,

I can confirm that the change fixed the segment fault issue I observed
on 4.16.1 release.

However, the behavior is still not entirely clean after the fix. But
that's probably a separate issue.
I have two devices assigned to pckback through kernel command-line.
When I attempted to use pci-assignable-remove on the SSD that caused
me a lot of trouble, it fails on the first attempt but works fine
later...
root@gaia:~# xl pci-assignable-list
0000:00:17.0
0000:05:00.0
root@gaia:~# xl pci-assignable-remove 05:00.0
libxl: error: libxl_pci.c:906:libxl__device_pci_assignable_remove:
failed to de-quarantine 0000:05:00.0
root@gaia:~# xl pci-assignable-list
0000:00:17.0
0000:05:00.0
root@gaia:~# xl pci-assignable-add 05:00.0
libxl: warning: libxl_pci.c:802:libxl__device_pci_assignable_add:
0000:05:00.0 already assigned to pciback
root@gaia:~# xl pci-assignable-list
0000:00:17.0
0000:05:00.0
root@gaia:~# xl pci-assignable-remove 05:00.0
root@gaia:~# xl pci-assignable-list
0000:00:17.0
root@gaia:~# xl pci-assignable-add 05:00.0
libxl: warning: libxl_pci.c:822:libxl__device_pci_assignable_add:
0000:05:00.0 not bound to a driver, will not be rebound.
root@gaia:~# xl pci-assignable-list
0000:00:17.0
0000:05:00.0

I'm no longer in a debug environment, so cannot collect more debug
info right now.

Thanks,
G.R.

On 11.07.2022 17:35, G.R. wrote:
> On Mon, Jul 11, 2022 at 7:03 PM Juergen Gross <jgross@suse.com> wrote:
>>
>> On 11.07.22 12:38, Anthony PERARD wrote:
>>> libxl__xs_directory() can potentially return NULL without setting `n`.
>>> As `n` isn't initialised, we need to check libxl__xs_directory()
>>> return value before checking `n`. Otherwise, `n` might be non-zero
>>> with `bdfs` NULL which would lead to a segv.
>>>
>>> Reported-by: "G.R." <firemeteor@users.sourceforge.net>
>>> Fixes: 57bff091f4 ("libxl: add 'name' field to 'libxl_device_pci' in the IDL...")
>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> I can confirm that the change fixed the segment fault issue I observed
> on 4.16.1 release.

I'll take the liberty and transform this into a Tested-by:.

Jan