On 06/27/2013 10:57 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Ensure that all APIs which list interface objects filter them against the access control system.
This makes the APIs for listing names and counting devices slightly less efficient, since we can't use the direct netcf APIs for these tasks. Instead we have to ask netcf for the full list of objects & iterate over the list filtering them out.
Such is life with security filtering.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/interface_conf.h | 3 + src/interface/interface_backend_netcf.c | 262 +++++++++++++++++++++++++++----- src/interface/interface_backend_udev.c | 56 +++++-- 3 files changed, 270 insertions(+), 51 deletions(-)
+++ b/src/interface/interface_backend_netcf.c @@ -209,48 +209,233 @@ static int netcfInterfaceClose(virConnectPtr conn) return 0; }
-static int netcfConnectNumOfInterfaces(virConnectPtr conn) +static int netcfConnectNumOfInterfacesImpl(virConnectPtr conn, + int status, + virInterfaceObjListFilter filter) { - int count; struct interface_driver *driver = conn->interfacePrivateData; + int count; + int want = 0; + int ret = -1; + int i; + char **names = NULL;
- if (virConnectNumOfInterfacesEnsureACL(conn) < 0) - return -1; - - interfaceDriverLock(driver); - count = ncf_num_of_interfaces(driver->netcf, NETCF_IFACE_ACTIVE); + /* List all interfaces, in case of we might support new filter flags + * except active|inactive in future.
s/case of/case/ s/except/beyond/
+ + if (VIR_ALLOC_N(names, count) < 0) { + virReportOOMError(); + goto cleanup; + }
[Uggh - we still need to revisit the goal to hoist OOM reporting into VIR_ALLOC and friends - nothing magic happened during my 3-week vacation on that front. But that's a completely separate project.]
+static int netcfConnectListInterfacesImpl(virConnectPtr conn, + int status, + char **const names, int nnames, + virInterfaceObjListFilter filter) { struct interface_driver *driver = conn->interfacePrivateData; - int count; + int count = 0; + int want = 0; + int ret = -1; + int i; + char **allnames = NULL;
- if (virConnectListInterfacesEnsureACL(conn) < 0) - return -1; + count = ncf_num_of_interfaces(driver->netcf, status);
Can any of this be shared? You have: long setup foreach element if not filtered count++ and long setup foreach element if not filtered append object where the long setup is the same. I think some of the other filtering code has used a single implementation, where passing in NULL for the **names parameter implies just counting, and passing in non-NULL implies object appending. Then you wouldn't be duplicating as much code, making it easier to add new filters in one place in the future.
+static int netcfConnectListInterfaces(virConnectPtr conn, char **const names, int nnames) +{ + struct interface_driver *driver = conn->interfacePrivateData; + int count; + + if (virConnectListInterfacesEnsureACL(conn) < 0) + return -1;
+ interfaceDriverLock(driver); + count = netcfConnectListInterfacesImpl(conn, + NETCF_IFACE_ACTIVE, + names, nnames, + virConnectListInterfacesCheckACL);
Indentation is off.
@@ -403,6 +575,17 @@ netcfConnectListAllInterfaces(virConnectPtr conn, goto cleanup; }
+ if (!(def = netcfGetMinimalDefForDevice(iface))) + goto cleanup; + + if (!virConnectListAllInterfacesCheckACL(conn, def)) { + ncf_if_free(iface); + iface = NULL; + virInterfaceDefFree(def); + continue; + } + virInterfaceDefFree(def); + /* XXX: Filter the result, need to be splitted once new filter flags * except active|inactive are supported.
Pre-existing, but as long as you are here, s/splitted/split/. Or maybe even nuke the comment, now that you have set up the framework for additional filtering so it no longer applies.
@@ -412,6 +595,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn, (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) && (status & NETCF_IFACE_INACTIVE)))) { ncf_if_free(iface); + iface = NULL; continue;
Oooh tricky - you posted the CVE-2013-2218 patch embedded inside this patch, prior to the embargo date, but no one picked up on it until now :) At any rate, a consolidation patch between the count vs. list function is probably worth a separate patch, at which point your code as-is looks mostly okay. ACK with grammar/spacing nits fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org