On 06/27/2013 10:57 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list