Re: [libvirt] [PATCH 2/4] remote: Check for limits when encoding typed params

The same way we check for limits when decoding typed parameters (virTypedParamsDeserialize()) we should do the same check when serializing them so that we don't put onto the wire more than our limits allow. Surprisingly, we were doing so explicitly in some places but not all of them. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/admin/admin_remote.c | 2 ++ src/admin/admin_server_dispatch.c | 27 ++------------- src/remote/remote_daemon_dispatch.c | 54 ++++++++++----------------- -- src/remote/remote_driver.c | 48 ++++--------------------- src/rpc/gendispatch.pl | 2 ++ src/util/virtypedparam.c | 13 ++++++- src/util/virtypedparam.h | 1 + 7 files changed, 45 insertions(+), 102 deletions(-) diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index e112c1f115..0846e72f6d 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -294,6 +294,7 @@ remoteAdminServerSetThreadPoolParameters(virAdmServerPtr srv, virObjectLock(priv); if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_THREADPOOL_PARAMETERS_M AX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, 0) < 0) @@ -405,6 +406,7 @@ remoteAdminServerSetClientLimits(virAdmServerPtr srv, virObjectLock(priv); if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_CLIENT_LIMITS_MAX, (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, 0) < 0) diff --git a/src/admin/admin_server_dispatch.c b/src/admin/admin_server_dispatch.c index 1973664488..3c4d72dedb 100644 --- a/src/admin/admin_server_dispatch.c +++ b/src/admin/admin_server_dispatch.c @@ -237,15 +237,8 @@ adminDispatchServerGetThreadpoolParameters(virNetServerPtr server ATTRIBUTE_UNUS args->flags) < 0) goto cleanup; - if (nparams > ADMIN_SERVER_THREADPOOL_PARAMETERS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of threadpool parameters %d exceeds max " - "allowed limit: %d"), nparams, - ADMIN_SERVER_THREADPOOL_PARAMETERS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_THREADPOOL_PARAMETERS_M AX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, 0) < 0) goto cleanup; @@ -336,15 +329,8 @@ adminDispatchClientGetInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (adminClientGetInfo(clnt, &params, &nparams, args->flags) < 0) goto cleanup; - if (nparams > ADMIN_CLIENT_INFO_PARAMETERS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of client info parameters %d exceeds max " - "allowed limit: %d"), nparams, - ADMIN_CLIENT_INFO_PARAMETERS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + ADMIN_CLIENT_INFO_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) @@ -383,15 +369,8 @@ adminDispatchServerGetClientLimits(virNetServerPtr server ATTRIBUTE_UNUSED, if (adminServerGetClientLimits(srv, &params, &nparams, args- >flags) < 0) goto cleanup; - if (nparams > ADMIN_SERVER_CLIENT_LIMITS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Number of client processing parameters %d exceeds " - "max allowed limit: %d"), nparams, - ADMIN_SERVER_CLIENT_LIMITS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + ADMIN_SERVER_CLIENT_LIMITS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, 0) < 0) goto cleanup; diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 1a002957ef..e205c743c3 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1152,6 +1152,7 @@ remoteRelayDomainEventTunable(virConnectPtr conn, goto error; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_EVENT_TUNABLE_MAX, (virTypedParameterRemotePtr *) &data.params.params_val, &data.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -1318,6 +1319,7 @@ remoteRelayDomainEventJobCompleted(virConnectPtr conn, goto error; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_JOB_STATS_MAX, (virTypedParameterRemotePtr *) &data.params.params_val, &data.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -2450,6 +2452,7 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS goto cleanup; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_SCHEDULER_PARAMETERS_M AX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, 0) < 0) @@ -2498,6 +2501,7 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE goto cleanup; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_SCHEDULER_PARAMETERS_M AX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, args->flags) < 0) @@ -2663,6 +2667,7 @@ remoteDispatchDomainBlockStatsFlags(virNetServerPtr server ATTRIBUTE_UNUSED, /* Serialize the block stats. */ if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_BLOCK_STATS_PARAMETERS _MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3292,6 +3297,7 @@ remoteDispatchDomainGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, } if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3351,6 +3357,7 @@ remoteDispatchDomainGetNumaParameters(virNetServerPtr server ATTRIBUTE_UNUSED, } if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_NUMA_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, flags) < 0) @@ -3410,6 +3417,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED, } if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3587,12 +3595,8 @@ remoteDispatchDomainGetLaunchSecurityInfo(virNetServerPtr server ATTRIBUTE_UNUSE if (virDomainGetLaunchSecurityInfo(dom, &params, &nparams, args- >flags) < 0) goto cleanup; - if (nparams > REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_P ARAMS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, args->flags) < 0) @@ -3631,12 +3635,8 @@ remoteDispatchDomainGetPerfEvents(virNetServerPtr server ATTRIBUTE_UNUSED, if (virDomainGetPerfEvents(dom, &params, &nparams, args->flags) < 0) goto cleanup; - if (nparams > REMOTE_DOMAIN_PERF_EVENTS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_PERF_EVENTS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, 0) < 0) @@ -3732,6 +3732,7 @@ remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED, /* Serialize the block I/O tuning parameters. */ if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETE RS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5289,6 +5290,7 @@ remoteDispatchDomainGetInterfaceParameters(virNetServerPtr server ATTRIBUTE_UNUS } if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_INTERFACE_PARAMETERS_M AX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, flags) < 0) @@ -5348,6 +5350,7 @@ remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, goto success; if (virTypedParamsSerialize(params, args->nparams * args->ncpus, + REMOTE_DOMAIN_GET_CPU_STATS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5450,13 +5453,8 @@ remoteDispatchNodeGetSevInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (virNodeGetSEVInfo(conn, &params, &nparams, args->flags) < 0) goto cleanup; - if (nparams > REMOTE_NODE_SEV_INFO_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - - if (virTypedParamsSerialize(params, nparams, + REMOTE_NODE_SEV_INFO_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5511,6 +5509,7 @@ remoteDispatchNodeGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, } if (virTypedParamsSerialize(params, nparams, + REMOTE_NODE_MEMORY_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, args->flags) < 0) @@ -5641,14 +5640,8 @@ remoteDispatchDomainGetJobStats(virNetServerPtr server ATTRIBUTE_UNUSED, &nparams, args->flags) < 0) goto cleanup; - if (nparams > REMOTE_DOMAIN_JOB_STATS_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many job stats '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_JOB_STATS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_JOB_STATS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, 0) < 0) @@ -7018,6 +7011,7 @@ remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED, if (virTypedParamsSerialize(retStats[i]->params, retStats[i]->nparams, + REMOTE_CONNECT_GET_ALL_DOMAI N_STATS_MAX, (virTypedParameterRemotePtr *) &dst->params.params_val, &dst->params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) @@ -7370,12 +7364,8 @@ remoteDispatchNetworkPortGetParameters(virNetServerPtr server ATTRIBUTE_UNUSED, if (virNetworkPortGetParameters(port, &params, &nparams, args- >flags) < 0) goto cleanup; - if (nparams > REMOTE_NETWORK_PORT_PARAMETERS_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_NETWORK_PORT_PARAMETERS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, args->flags) < 0) @@ -7674,14 +7664,8 @@ remoteDispatchDomainGetGuestInfo(virNetServerPtr server ATTRIBUTE_UNUSED, if (virDomainGetGuestInfo(dom, args->types, &params, &nparams, args->flags) < 0) goto cleanup; - if (nparams > REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many params in guestinfo: %d for limit %d"), - nparams, REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX, (virTypedParameterRemotePtr *) &ret- >params.params_val, &ret->params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8cc7ab3663..2b86f55035 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7109,14 +7109,8 @@ remoteDomainMigrateBegin3Params(virDomainPtr domain, make_nonnull_domain(&args.dom, domain); args.flags = flags; - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7177,14 +7171,8 @@ remoteDomainMigratePrepare3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7261,18 +7249,12 @@ remoteDomainMigratePrepareTunnel3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.flags = flags; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7351,13 +7333,6 @@ remoteDomainMigratePerform3Params(virDomainPtr dom, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - make_nonnull_domain(&args.dom, dom); args.dconnuri = dconnuri == NULL ? NULL : (char **) &dconnuri; args.cookie_in.cookie_in_val = (char *)cookiein; @@ -7365,6 +7340,7 @@ remoteDomainMigratePerform3Params(virDomainPtr dom, args.flags = flags; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7425,19 +7401,13 @@ remoteDomainMigrateFinish3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.flags = flags; args.cancelled = cancelled; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { @@ -7499,13 +7469,6 @@ remoteDomainMigrateConfirm3Params(virDomainPtr domain, memset(&args, 0, sizeof(args)); - if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many migration parameters '%d' for limit '%d'"), - nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); - goto cleanup; - } - make_nonnull_domain(&args.dom, domain); args.cookie_in.cookie_in_len = cookieinlen; args.cookie_in.cookie_in_val = (char *) cookiein; @@ -7513,6 +7476,7 @@ remoteDomainMigrateConfirm3Params(virDomainPtr domain, args.cancelled = cancelled; if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX , (virTypedParameterRemotePtr *) &args.params.params_val, &args.params.params_len, VIR_TYPED_PARAM_STRING_OKAY) < 0) { diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index ff21834091..9d3cc5b145 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -951,6 +951,7 @@ elsif ($mode eq "server") { splice(@args_list, int($5), 0, "&$1_len"); push(@ret_list, "if (virTypedParamsSerialize($1, $1_len,\n" . + " $2,\n" . " (virTypedParameterRemotePtr *) &ret->$1.$1_val,\n" . " &ret->$1.$1_len,\n" . " VIR_TYPED_PARAM_STRING_OKAY) < 0)\n" . @@ -1436,6 +1437,7 @@ elsif ($mode eq "client") { push(@args_list, "virTypedParameterPtr $1"); push(@args_list, "int n$1"); push(@setters_list2, "if (virTypedParamsSerialize($1, n$1,\n" . + " $2,\n" . " (virTypedParameterRemotePtr *) &args.$1.$1_val,\n" . " &args.$1.$1_len,\n" . " VIR_TYPED_PARAM_STRING_OKAY) < 0) {\n" . diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 8f23348d97..779714d146 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1480,12 +1480,15 @@ virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params, * virTypedParamsSerialize: * @params: array of parameters to be serialized and later sent to remote side * @nparams: number of elements in @params + * @limit: user specified maximum limit to @remote_params_len * @remote_params_val: protocol independent remote representation of @params * @remote_params_len: the final number of elements in @remote_params_val * @flags: bitwise-OR of virTypedParameterFlags * * This method serializes typed parameters provided by @params into * @remote_params_val which is the representation actually being sent. + * It also checks, if the @limit imposed by RPC on the maximum number of + * parameters is not exceeded. * * Server side using this method also filters out any string parameters that * must not be returned to older clients and handles possibly sparse arrays @@ -1496,6 +1499,7 @@ virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params, int virTypedParamsSerialize(virTypedParameterPtr params, int nparams, + int limit, virTypedParameterRemotePtr *remote_params_val, unsigned int *remote_params_len, unsigned int flags) @@ -1503,9 +1507,16 @@ virTypedParamsSerialize(virTypedParameterPtr params, size_t i; size_t j; int rv = -1; - virTypedParameterRemotePtr params_val; + virTypedParameterRemotePtr params_val = NULL; int params_len = nparams; + if (nparams > limit) { + virReportError(VIR_ERR_RPC, + _("too many parameters '%d' for limit '%d'"), + nparams, limit); + goto cleanup; + } + if (VIR_ALLOC_N(params_val, nparams) < 0) goto cleanup; diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index ce4e75b389..f9d22b24fb 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -113,6 +113,7 @@ int virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params, int virTypedParamsSerialize(virTypedParameterPtr params, int nparams, + int limit, virTypedParameterRemotePtr *remote_params_val, unsigned int *remote_params_len, unsigned int flags);