| Message ID | 20220711103847.21276-1-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN] libxl: Check return value of libxl__xs_directory in name2bdf | expand |
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
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++) {
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(-)