| Message ID | 1323230623-8709-1-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 12/07/2011 06:03 AM, Michael Roth wrote: > This adds a command-line option, -b/--blacklist, that accepts a > comma-seperated list of RPCs to disable, or prints a list of > available RPCs if passed "?". > > In consequence this also adds general blacklisting and RPC listing > facilities to the new QMP dispatch/registry facilities, should the > QMP monitor ever have a need for such a thing. Beyond run time disablement, how easy it is to compile out some of the general commands such as exec/file-handling? Security certifications like common criteria usually ask to compile out anything that might tamper security. > > Ideally, to avoid support/compatability issues in the future, > blacklisting guest agent functionality will be the exceptional > case, but we add the functionality here to handle guest administrators > with specific requirements. > > Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com> > --- > qapi/qmp-core.h | 3 +++ > qapi/qmp-dispatch.c | 4 ++++ > qapi/qmp-registry.c | 43 ++++++++++++++++++++++++++++++++++++++----- > qemu-ga.c | 37 ++++++++++++++++++++++++++++++++++--- > qerror.c | 4 ++++ > qerror.h | 3 +++ > 6 files changed, 86 insertions(+), 8 deletions(-) > > diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h > index f1c26e4..3cf1781 100644 > --- a/qapi/qmp-core.h > +++ b/qapi/qmp-core.h > @@ -31,11 +31,14 @@ typedef struct QmpCommand > QmpCommandType type; > QmpCommandFunc *fn; > QTAILQ_ENTRY(QmpCommand) node; > + bool enabled; > } QmpCommand; > > void qmp_register_command(const char *name, QmpCommandFunc *fn); > QmpCommand *qmp_find_command(const char *name); > QObject *qmp_dispatch(QObject *request); > +void qmp_disable_command(const char *name); > +char **qmp_get_command_list(void); > > #endif > > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c > index 5584693..43f640a 100644 > --- a/qapi/qmp-dispatch.c > +++ b/qapi/qmp-dispatch.c > @@ -79,6 +79,10 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp) > error_set(errp, QERR_COMMAND_NOT_FOUND, command); > return NULL; > } > + if (!cmd->enabled) { > + error_set(errp, QERR_COMMAND_DISABLED, command); > + return NULL; > + } > > if (!qdict_haskey(dict, "arguments")) { > args = qdict_new(); > diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c > index 5ff99cf..abafa34 100644 > --- a/qapi/qmp-registry.c > +++ b/qapi/qmp-registry.c > @@ -14,7 +14,7 @@ > > #include "qapi/qmp-core.h" > > -static QTAILQ_HEAD(, QmpCommand) qmp_commands = > +static QTAILQ_HEAD(QmpCommandList, QmpCommand) qmp_commands = > QTAILQ_HEAD_INITIALIZER(qmp_commands); > > void qmp_register_command(const char *name, QmpCommandFunc *fn) > @@ -24,17 +24,50 @@ void qmp_register_command(const char *name, QmpCommandFunc *fn) > cmd->name = name; > cmd->type = QCT_NORMAL; > cmd->fn = fn; > + cmd->enabled = true; > QTAILQ_INSERT_TAIL(&qmp_commands, cmd, node); > } > > QmpCommand *qmp_find_command(const char *name) > { > - QmpCommand *i; > + QmpCommand *cmd; > > - QTAILQ_FOREACH(i,&qmp_commands, node) { > - if (strcmp(i->name, name) == 0) { > - return i; > + QTAILQ_FOREACH(cmd,&qmp_commands, node) { > + if (strcmp(cmd->name, name) == 0) { > + return cmd; > } > } > return NULL; > } > + > +void qmp_disable_command(const char *name) > +{ > + QmpCommand *cmd; > + > + QTAILQ_FOREACH(cmd,&qmp_commands, node) { > + if (strcmp(cmd->name, name) == 0) { > + cmd->enabled = false; > + return; > + } > + } > +} > + > +char **qmp_get_command_list(void) > +{ > + QmpCommand *cmd; > + int count = 1; > + char **list_head, **list; > + > + QTAILQ_FOREACH(cmd,&qmp_commands, node) { > + count++; > + } > + > + list_head = list = g_malloc0(count * sizeof(char *)); > + > + QTAILQ_FOREACH(cmd,&qmp_commands, node) { > + *list = strdup(cmd->name); > + list++; > + } > + > + return list_head; > +} > diff --git a/qemu-ga.c b/qemu-ga.c > index 4932013..200bb15 100644 > --- a/qemu-ga.c > +++ b/qemu-ga.c > @@ -27,6 +27,7 @@ > #include "signal.h" > #include "qerror.h" > #include "error_int.h" > +#include "qapi/qmp-core.h" > > #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0" > #define QGA_PIDFILE_DEFAULT "/var/run/qemu-ga.pid" > @@ -91,6 +92,8 @@ static void usage(const char *cmd) > " -v, --verbose log extra debugging information\n" > " -V, --version print version information and exit\n" > " -d, --daemonize become a daemon\n" > +" -b, --blacklist comma-seperated list of RPCs to disable (no spaces, \"?\"" > +" to list available RPCs)\n" > " -h, --help display this help and exit\n" > "\n" > "Report bugs to<mdroth@linux.vnet.ibm.com>\n" > @@ -548,7 +551,7 @@ static void init_guest_agent(GAState *s) > > int main(int argc, char **argv) > { > - const char *sopt = "hVvdm:p:l:f:"; > + const char *sopt = "hVvdm:p:l:f:b:"; > const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT; > const struct option lopt[] = { > { "help", 0, NULL, 'h' }, > @@ -559,13 +562,16 @@ int main(int argc, char **argv) > { "method", 0, NULL, 'm' }, > { "path", 0, NULL, 'p' }, > { "daemonize", 0, NULL, 'd' }, > + { "blacklist", 0, NULL, 'b' }, > { NULL, 0, NULL, 0 } > }; > - int opt_ind = 0, ch, daemonize = 0; > + int opt_ind = 0, ch, daemonize = 0, i, j, len; > GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL; > FILE *log_file = stderr; > GAState *s; > > + module_call_init(MODULE_INIT_QAPI); > + > while ((ch = getopt_long(argc, argv, sopt, lopt,&opt_ind)) != -1) { > switch (ch) { > case 'm': > @@ -595,6 +601,32 @@ int main(int argc, char **argv) > case 'd': > daemonize = 1; > break; > + case 'b': { > + char **list_head, **list; > + if (*optarg == '?') { > + list_head = list = qmp_get_command_list(); > + while (*list != NULL) { > + printf("%s\n", *list); > + g_free(*list); > + list++; > + } > + g_free(list_head); > + return 0; > + } > + for (j = 0, i = 0, len = strlen(optarg); i< len; i++) { > + if (optarg[i] == ',') { > + optarg[i] = 0; > + qmp_disable_command(&optarg[j]); > + g_debug("disabling command: %s",&optarg[j]); > + j = i + 1; > + } > + } > + if (j< i) { > + qmp_disable_command(&optarg[j]); > + g_debug("disabling command: %s",&optarg[j]); > + } > + break; > + } > case 'h': > usage(argv[0]); > return 0; > @@ -624,7 +656,6 @@ int main(int argc, char **argv) > ga_command_state_init_all(s->command_state); > ga_state = s; > > - module_call_init(MODULE_INIT_QAPI); > init_guest_agent(ga_state); > register_signal_handlers(); > > diff --git a/qerror.c b/qerror.c > index 656efc2..a998d2f 100644 > --- a/qerror.c > +++ b/qerror.c > @@ -65,6 +65,10 @@ static const QErrorStringTable qerror_table[] = { > .desc = "The command %(name) has not been found", > }, > { > + .error_fmt = QERR_COMMAND_DISABLED, > + .desc = "The command %(name) has been disabled for this instance", > + }, > + { > .error_fmt = QERR_DEVICE_ENCRYPTED, > .desc = "Device '%(device)' is encrypted", > }, > diff --git a/qerror.h b/qerror.h > index 161d654..0d3d4c5 100644 > --- a/qerror.h > +++ b/qerror.h > @@ -66,6 +66,9 @@ QError *qobject_to_qerror(const QObject *obj); > #define QERR_COMMAND_NOT_FOUND \ > "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }" > > +#define QERR_COMMAND_DISABLED \ > + "{ 'class': 'CommandDisabled', 'data': { 'name': %s } }" > + > #define QERR_DEVICE_ENCRYPTED \ > "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }" >
On Wed, Dec 07, 2011 at 12:34:01PM +0200, Dor Laor wrote: > On 12/07/2011 06:03 AM, Michael Roth wrote: > >This adds a command-line option, -b/--blacklist, that accepts a > >comma-seperated list of RPCs to disable, or prints a list of > >available RPCs if passed "?". > > > >In consequence this also adds general blacklisting and RPC listing > >facilities to the new QMP dispatch/registry facilities, should the > >QMP monitor ever have a need for such a thing. > > Beyond run time disablement, how easy it is to compile out some of > the general commands such as exec/file-handling? > > Security certifications like common criteria usually ask to compile > out anything that might tamper security. I don't think that's really relevant/needed. As discussed on the call yesterday, this is security theatre, because nothing can prevent the host admin from accessing guest RAM or disk data. AFAIK the virtualization related security certifications acknowledge this already & don't make any claims about security of guests against a malicious host admin. In any case, a suitable SELinux policy for the guest agent could prevent arbitrary file/binary access via generic 'exec' / 'file-read' commands, in a manner that is sufficient to satisfy security certications. Regards, Daniel
On 12/07/2011 12:52 PM, Daniel P. Berrange wrote: > On Wed, Dec 07, 2011 at 12:34:01PM +0200, Dor Laor wrote: >> On 12/07/2011 06:03 AM, Michael Roth wrote: >>> This adds a command-line option, -b/--blacklist, that accepts a >>> comma-seperated list of RPCs to disable, or prints a list of >>> available RPCs if passed "?". >>> >>> In consequence this also adds general blacklisting and RPC listing >>> facilities to the new QMP dispatch/registry facilities, should the >>> QMP monitor ever have a need for such a thing. >> >> Beyond run time disablement, how easy it is to compile out some of >> the general commands such as exec/file-handling? >> >> Security certifications like common criteria usually ask to compile >> out anything that might tamper security. > > I don't think that's really relevant/needed. As discussed on the > call yesterday, this is security theatre, because nothing can prevent > the host admin from accessing guest RAM or disk data. AFAIK the > virtualization related security certifications acknowledge this > already& don't make any claims about security of guests against > a malicious host admin. In any case, a suitable SELinux policy for > the guest agent could prevent arbitrary file/binary access via > generic 'exec' / 'file-read' commands, in a manner that is sufficient > to satisfy security certications. I absolutely agree that the hypervisor can tweak the guest in multiple ways. Nevertheless there are two reasons I asked it: 1. Reduce code and noise from security reviewers eyes. We were asked to do exactly that for other qemu functionality that is included but does not run at all. It's just makes the review faster. 2. Every piece of code is a risk for exploit Imagine that a bug/leak/use-after-free in the blacklist command or the exec command on qemu exists and allows attacked to gain control of qemu. > > Regards, > Daniel
On 12/07/2011 06:12 AM, Dor Laor wrote: > On 12/07/2011 12:52 PM, Daniel P. Berrange wrote: >> On Wed, Dec 07, 2011 at 12:34:01PM +0200, Dor Laor wrote: >>> On 12/07/2011 06:03 AM, Michael Roth wrote: >>>> This adds a command-line option, -b/--blacklist, that accepts a >>>> comma-seperated list of RPCs to disable, or prints a list of >>>> available RPCs if passed "?". >>>> >>>> In consequence this also adds general blacklisting and RPC listing >>>> facilities to the new QMP dispatch/registry facilities, should the >>>> QMP monitor ever have a need for such a thing. >>> >>> Beyond run time disablement, how easy it is to compile out some of >>> the general commands such as exec/file-handling? >>> >>> Security certifications like common criteria usually ask to compile >>> out anything that might tamper security. >> >> I don't think that's really relevant/needed. As discussed on the >> call yesterday, this is security theatre, because nothing can prevent >> the host admin from accessing guest RAM or disk data. AFAIK the >> virtualization related security certifications acknowledge this >> already& don't make any claims about security of guests against >> a malicious host admin. In any case, a suitable SELinux policy for >> the guest agent could prevent arbitrary file/binary access via >> generic 'exec' / 'file-read' commands, in a manner that is sufficient >> to satisfy security certications. > > I absolutely agree that the hypervisor can tweak the guest in multiple > ways. Nevertheless there are two reasons I asked it: > > 1. Reduce code and noise from security reviewers eyes. > We were asked to do exactly that for other qemu functionality that > is included but does not run at all. It's just makes the review > faster. Actually removing the code, or compiling it out? If it's a matter of compiling it out, the best solution I can think of is having the QAPI code generators create a #define <rpc> for each RPC, then wrapping the implementations inside an #ifdef <rpc>. That way you could compile out the code by simply modifying the schema. That said, I'd really like to avoid having distros get into the habit of extensively modifying their guest agent source outside of bug fixes and whatnot, I think it'll cause too many problems down the road. From a management perspective, if you're running a cloud with multiple distros, it'll be really difficult to account for agents that have been modified or crippled in various ways. Perhaps we only need, say, shutdown, for ovirt, and compile out the rest, but maybe a customer wants to run their RHEL guest in home-brewed environment where they use qemu-ga file read/write to handle a specific set of guest activation procedures. Now they need a new agent package. It's a whole lot of hassle for host/guest admins for the sake of saving a security reviewer a bit of investigating that'll lead right back to the general operating premise that you have to trust your host administrators before any chain of trust can be established. At least with this interface we can provide some semblance of relief to users with specific security concerns, but don't have to work with distros to re-package agents when those concerns collide with requirements on the host side. We can just check to see if they disabled the functionality and request they re-enable due to <reason> by updating their configs. > > 2. Every piece of code is a risk for exploit > Imagine that a bug/leak/use-after-free in the blacklist command or > the exec command on qemu exists and allows attacked to gain control > of qemu. A host can never assume that a guest [agent] can be trusted. qemu-ga might've been replaced completely by a malicious guest admin, thus circumventing any steps a distro has taken to harden it. Fortunately a guest can only affect memory outside it's address space by going through the virtio-serial/QMP layer. So we can focus our efforts on hardening the transport and json parser layers, and a lot of work has gone into that already (placing limits on token size, recursion depth, etc). So that's more an issue that needs to be addressed on the qemu side, and is independent of any particular RPC implementation on the guest side. > >> >> Regards, >> Daniel >
On 12/07/2011 06:45 PM, Michael Roth wrote: > On 12/07/2011 06:12 AM, Dor Laor wrote: >> On 12/07/2011 12:52 PM, Daniel P. Berrange wrote: >>> On Wed, Dec 07, 2011 at 12:34:01PM +0200, Dor Laor wrote: >>>> On 12/07/2011 06:03 AM, Michael Roth wrote: >>>>> This adds a command-line option, -b/--blacklist, that accepts a >>>>> comma-seperated list of RPCs to disable, or prints a list of >>>>> available RPCs if passed "?". >>>>> >>>>> In consequence this also adds general blacklisting and RPC listing >>>>> facilities to the new QMP dispatch/registry facilities, should the >>>>> QMP monitor ever have a need for such a thing. >>>> >>>> Beyond run time disablement, how easy it is to compile out some of >>>> the general commands such as exec/file-handling? >>>> >>>> Security certifications like common criteria usually ask to compile >>>> out anything that might tamper security. >>> >>> I don't think that's really relevant/needed. As discussed on the >>> call yesterday, this is security theatre, because nothing can prevent >>> the host admin from accessing guest RAM or disk data. AFAIK the >>> virtualization related security certifications acknowledge this >>> already& don't make any claims about security of guests against >>> a malicious host admin. In any case, a suitable SELinux policy for >>> the guest agent could prevent arbitrary file/binary access via >>> generic 'exec' / 'file-read' commands, in a manner that is sufficient >>> to satisfy security certications. >> >> I absolutely agree that the hypervisor can tweak the guest in multiple >> ways. Nevertheless there are two reasons I asked it: >> >> 1. Reduce code and noise from security reviewers eyes. >> We were asked to do exactly that for other qemu functionality that >> is included but does not run at all. It's just makes the review >> faster. > > Actually removing the code, or compiling it out? > > If it's a matter of compiling it out, the best solution I can think of > is having the QAPI code generators create a #define <rpc> for each RPC, > then wrapping the implementations inside an #ifdef <rpc>. That way you > could compile out the code by simply modifying the schema. > > That said, I'd really like to avoid having distros get into the habit of > extensively modifying their guest agent source outside of bug fixes and > whatnot, I think it'll cause too many problems down the road. From a > management perspective, if you're running a cloud with multiple distros, > it'll be really difficult to account for agents that have been modified > or crippled in various ways. I don't mind ignoring the guest side for security issues, but since we're discussing it, isn't the mechanism for capability exchange will take of command existence? We'll need it anyway to handle various agent versions. > > Perhaps we only need, say, shutdown, for ovirt, and compile out the > rest, but maybe a customer wants to run their RHEL guest in home-brewed > environment where they use qemu-ga file read/write to handle a specific > set of guest activation procedures. Now they need a new agent package. > > It's a whole lot of hassle for host/guest admins for the sake of saving > a security reviewer a bit of investigating that'll lead right back to > the general operating premise that you have to trust your host > administrators before any chain of trust can be established. > > At least with this interface we can provide some semblance of relief to > users with specific security concerns, but don't have to work with > distros to re-package agents when those concerns collide with > requirements on the host side. We can just check to see if they disabled > the functionality and request they re-enable due to <reason> by updating > their configs. > >> >> 2. Every piece of code is a risk for exploit >> Imagine that a bug/leak/use-after-free in the blacklist command or >> the exec command on qemu exists and allows attacked to gain control >> of qemu. > > A host can never assume that a guest [agent] can be trusted. qemu-ga > might've been replaced completely by a malicious guest admin, thus > circumventing any steps a distro has taken to harden it. Fortunately a > guest can only affect memory outside it's address space by going through > the virtio-serial/QMP layer. So we can focus our efforts on hardening > the transport and json parser layers, and a lot of work has gone into > that already (placing limits on token size, recursion depth, etc). So > that's more an issue that needs to be addressed on the qemu side, and is > independent of any particular RPC implementation on the guest side. I agree that the host side is relevant and this guest side is negligible here since a malicious guest will create its own agent. > >> >>> >>> Regards, >>> Daniel >> >
On 12/08/2011 04:53 PM, Dor Laor wrote: > On 12/07/2011 06:45 PM, Michael Roth wrote: >> On 12/07/2011 06:12 AM, Dor Laor wrote: >>> On 12/07/2011 12:52 PM, Daniel P. Berrange wrote: >>>> On Wed, Dec 07, 2011 at 12:34:01PM +0200, Dor Laor wrote: >>>>> On 12/07/2011 06:03 AM, Michael Roth wrote: >>>>>> This adds a command-line option, -b/--blacklist, that accepts a >>>>>> comma-seperated list of RPCs to disable, or prints a list of >>>>>> available RPCs if passed "?". >>>>>> >>>>>> In consequence this also adds general blacklisting and RPC listing >>>>>> facilities to the new QMP dispatch/registry facilities, should the >>>>>> QMP monitor ever have a need for such a thing. >>>>> >>>>> Beyond run time disablement, how easy it is to compile out some of >>>>> the general commands such as exec/file-handling? >>>>> >>>>> Security certifications like common criteria usually ask to compile >>>>> out anything that might tamper security. >>>> >>>> I don't think that's really relevant/needed. As discussed on the >>>> call yesterday, this is security theatre, because nothing can prevent >>>> the host admin from accessing guest RAM or disk data. AFAIK the >>>> virtualization related security certifications acknowledge this >>>> already& don't make any claims about security of guests against >>>> a malicious host admin. In any case, a suitable SELinux policy for >>>> the guest agent could prevent arbitrary file/binary access via >>>> generic 'exec' / 'file-read' commands, in a manner that is sufficient >>>> to satisfy security certications. >>> >>> I absolutely agree that the hypervisor can tweak the guest in multiple >>> ways. Nevertheless there are two reasons I asked it: >>> >>> 1. Reduce code and noise from security reviewers eyes. >>> We were asked to do exactly that for other qemu functionality that >>> is included but does not run at all. It's just makes the review >>> faster. >> >> Actually removing the code, or compiling it out? >> >> If it's a matter of compiling it out, the best solution I can think of >> is having the QAPI code generators create a #define <rpc> for each RPC, >> then wrapping the implementations inside an #ifdef <rpc>. That way you >> could compile out the code by simply modifying the schema. >> >> That said, I'd really like to avoid having distros get into the habit of >> extensively modifying their guest agent source outside of bug fixes and >> whatnot, I think it'll cause too many problems down the road. From a >> management perspective, if you're running a cloud with multiple distros, >> it'll be really difficult to account for agents that have been modified >> or crippled in various ways. > > I don't mind ignoring the guest side for security issues, but since > we're discussing it, isn't the mechanism for capability exchange will > take of command existence? We'll need it anyway to handle various agent > versions. Agreed, and with the capabilities reporting introduced in patch 2 we'd be able to determine whether a guest command was simply disabled, or if it was compiled out. So that's not too much a concern. The issue is that the latter case is much easier to rectify if the disabled command becomes a requirement on the host side, since it's a guest config change, rather than a re-spin of a guest agent package. For a homogenous environment, re-spinning the agent package isn't too difficult to deal with, but in a mixed environment there would be a lot of inertia in needing to coordinate requirements with multiple distro package maintainers to support new agent features and provide updated packages. The only way to get around this, for mixed environments, is if our primary deployment model is to support the agent for a number of distros (RHEL/SLES/etc) and have the host push new versions as needed (via ISO, or unattended via guest distro-packaged agent with remote update support). That way, each host/distro can push an agent that suites their specific requirements, while an upstream/community-supported guest tools ISO focuses on broader functionality. Kind of like the virtio-win drivers, where non-RHEL users can consume via community-supported unsigned drivers. But that still requires certain agent functionality to remain "off-limits", such as remote update (which is currently possible via guest-file-write and guest-shutdown, or eventually without shutdown via guest-exec, though a specific update interface would probably be warranted for this scenario). > >> >> Perhaps we only need, say, shutdown, for ovirt, and compile out the >> rest, but maybe a customer wants to run their RHEL guest in home-brewed >> environment where they use qemu-ga file read/write to handle a specific >> set of guest activation procedures. Now they need a new agent package. >> >> It's a whole lot of hassle for host/guest admins for the sake of saving >> a security reviewer a bit of investigating that'll lead right back to >> the general operating premise that you have to trust your host >> administrators before any chain of trust can be established. >> >> At least with this interface we can provide some semblance of relief to >> users with specific security concerns, but don't have to work with >> distros to re-package agents when those concerns collide with >> requirements on the host side. We can just check to see if they disabled >> the functionality and request they re-enable due to <reason> by updating >> their configs. >> >>> >>> 2. Every piece of code is a risk for exploit >>> Imagine that a bug/leak/use-after-free in the blacklist command or >>> the exec command on qemu exists and allows attacked to gain control >>> of qemu. >> >> A host can never assume that a guest [agent] can be trusted. qemu-ga >> might've been replaced completely by a malicious guest admin, thus >> circumventing any steps a distro has taken to harden it. Fortunately a >> guest can only affect memory outside it's address space by going through >> the virtio-serial/QMP layer. So we can focus our efforts on hardening >> the transport and json parser layers, and a lot of work has gone into >> that already (placing limits on token size, recursion depth, etc). So >> that's more an issue that needs to be addressed on the qemu side, and is >> independent of any particular RPC implementation on the guest side. > > I agree that the host side is relevant and this guest side is negligible > here since a malicious guest will create its own agent. > >> >>> >>>> >>>> Regards, >>>> Daniel >>> >> >
On 12/06/2011 10:03 PM, Michael Roth wrote: > This adds a command-line option, -b/--blacklist, that accepts a > comma-seperated list of RPCs to disable, or prints a list of > available RPCs if passed "?". > > In consequence this also adds general blacklisting and RPC listing > facilities to the new QMP dispatch/registry facilities, should the > QMP monitor ever have a need for such a thing. > > Ideally, to avoid support/compatability issues in the future, > blacklisting guest agent functionality will be the exceptional > case, but we add the functionality here to handle guest administrators > with specific requirements. > > Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com> Applied. Thanks. Regards, Anthony Liguori > --- > qapi/qmp-core.h | 3 +++ > qapi/qmp-dispatch.c | 4 ++++ > qapi/qmp-registry.c | 43 ++++++++++++++++++++++++++++++++++++++----- > qemu-ga.c | 37 ++++++++++++++++++++++++++++++++++--- > qerror.c | 4 ++++ > qerror.h | 3 +++ > 6 files changed, 86 insertions(+), 8 deletions(-) > > diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h > index f1c26e4..3cf1781 100644 > --- a/qapi/qmp-core.h > +++ b/qapi/qmp-core.h > @@ -31,11 +31,14 @@ typedef struct QmpCommand > QmpCommandType type; > QmpCommandFunc *fn; > QTAILQ_ENTRY(QmpCommand) node; > + bool enabled; > } QmpCommand; > > void qmp_register_command(const char *name, QmpCommandFunc *fn); > QmpCommand *qmp_find_command(const char *name); > QObject *qmp_dispatch(QObject *request); > +void qmp_disable_command(const char *name); > +char **qmp_get_command_list(void); > > #endif > > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c > index 5584693..43f640a 100644 > --- a/qapi/qmp-dispatch.c > +++ b/qapi/qmp-dispatch.c > @@ -79,6 +79,10 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp) > error_set(errp, QERR_COMMAND_NOT_FOUND, command); > return NULL; > } > + if (!cmd->enabled) { > + error_set(errp, QERR_COMMAND_DISABLED, command); > + return NULL; > + } > > if (!qdict_haskey(dict, "arguments")) { > args = qdict_new(); > diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c > index 5ff99cf..abafa34 100644 > --- a/qapi/qmp-registry.c > +++ b/qapi/qmp-registry.c > @@ -14,7 +14,7 @@ > > #include "qapi/qmp-core.h" > > -static QTAILQ_HEAD(, QmpCommand) qmp_commands = > +static QTAILQ_HEAD(QmpCommandList, QmpCommand) qmp_commands = > QTAILQ_HEAD_INITIALIZER(qmp_commands); > > void qmp_register_command(const char *name, QmpCommandFunc *fn) > @@ -24,17 +24,50 @@ void qmp_register_command(const char *name, QmpCommandFunc *fn) > cmd->name = name; > cmd->type = QCT_NORMAL; > cmd->fn = fn; > + cmd->enabled = true; > QTAILQ_INSERT_TAIL(&qmp_commands, cmd, node); > } > > QmpCommand *qmp_find_command(const char *name) > { > - QmpCommand *i; > + QmpCommand *cmd; > > - QTAILQ_FOREACH(i,&qmp_commands, node) { > - if (strcmp(i->name, name) == 0) { > - return i; > + QTAILQ_FOREACH(cmd,&qmp_commands, node) { > + if (strcmp(cmd->name, name) == 0) { > + return cmd; > } > } > return NULL; > } > + > +void qmp_disable_command(const char *name) > +{ > + QmpCommand *cmd; > + > + QTAILQ_FOREACH(cmd,&qmp_commands, node) { > + if (strcmp(cmd->name, name) == 0) { > + cmd->enabled = false; > + return; > + } > + } > +} > + > +char **qmp_get_command_list(void) > +{ > + QmpCommand *cmd; > + int count = 1; > + char **list_head, **list; > + > + QTAILQ_FOREACH(cmd,&qmp_commands, node) { > + count++; > + } > + > + list_head = list = g_malloc0(count * sizeof(char *)); > + > + QTAILQ_FOREACH(cmd,&qmp_commands, node) { > + *list = strdup(cmd->name); > + list++; > + } > + > + return list_head; > +} > diff --git a/qemu-ga.c b/qemu-ga.c > index 4932013..200bb15 100644 > --- a/qemu-ga.c > +++ b/qemu-ga.c > @@ -27,6 +27,7 @@ > #include "signal.h" > #include "qerror.h" > #include "error_int.h" > +#include "qapi/qmp-core.h" > > #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0" > #define QGA_PIDFILE_DEFAULT "/var/run/qemu-ga.pid" > @@ -91,6 +92,8 @@ static void usage(const char *cmd) > " -v, --verbose log extra debugging information\n" > " -V, --version print version information and exit\n" > " -d, --daemonize become a daemon\n" > +" -b, --blacklist comma-seperated list of RPCs to disable (no spaces, \"?\"" > +" to list available RPCs)\n" > " -h, --help display this help and exit\n" > "\n" > "Report bugs to<mdroth@linux.vnet.ibm.com>\n" > @@ -548,7 +551,7 @@ static void init_guest_agent(GAState *s) > > int main(int argc, char **argv) > { > - const char *sopt = "hVvdm:p:l:f:"; > + const char *sopt = "hVvdm:p:l:f:b:"; > const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT; > const struct option lopt[] = { > { "help", 0, NULL, 'h' }, > @@ -559,13 +562,16 @@ int main(int argc, char **argv) > { "method", 0, NULL, 'm' }, > { "path", 0, NULL, 'p' }, > { "daemonize", 0, NULL, 'd' }, > + { "blacklist", 0, NULL, 'b' }, > { NULL, 0, NULL, 0 } > }; > - int opt_ind = 0, ch, daemonize = 0; > + int opt_ind = 0, ch, daemonize = 0, i, j, len; > GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL; > FILE *log_file = stderr; > GAState *s; > > + module_call_init(MODULE_INIT_QAPI); > + > while ((ch = getopt_long(argc, argv, sopt, lopt,&opt_ind)) != -1) { > switch (ch) { > case 'm': > @@ -595,6 +601,32 @@ int main(int argc, char **argv) > case 'd': > daemonize = 1; > break; > + case 'b': { > + char **list_head, **list; > + if (*optarg == '?') { > + list_head = list = qmp_get_command_list(); > + while (*list != NULL) { > + printf("%s\n", *list); > + g_free(*list); > + list++; > + } > + g_free(list_head); > + return 0; > + } > + for (j = 0, i = 0, len = strlen(optarg); i< len; i++) { > + if (optarg[i] == ',') { > + optarg[i] = 0; > + qmp_disable_command(&optarg[j]); > + g_debug("disabling command: %s",&optarg[j]); > + j = i + 1; > + } > + } > + if (j< i) { > + qmp_disable_command(&optarg[j]); > + g_debug("disabling command: %s",&optarg[j]); > + } > + break; > + } > case 'h': > usage(argv[0]); > return 0; > @@ -624,7 +656,6 @@ int main(int argc, char **argv) > ga_command_state_init_all(s->command_state); > ga_state = s; > > - module_call_init(MODULE_INIT_QAPI); > init_guest_agent(ga_state); > register_signal_handlers(); > > diff --git a/qerror.c b/qerror.c > index 656efc2..a998d2f 100644 > --- a/qerror.c > +++ b/qerror.c > @@ -65,6 +65,10 @@ static const QErrorStringTable qerror_table[] = { > .desc = "The command %(name) has not been found", > }, > { > + .error_fmt = QERR_COMMAND_DISABLED, > + .desc = "The command %(name) has been disabled for this instance", > + }, > + { > .error_fmt = QERR_DEVICE_ENCRYPTED, > .desc = "Device '%(device)' is encrypted", > }, > diff --git a/qerror.h b/qerror.h > index 161d654..0d3d4c5 100644 > --- a/qerror.h > +++ b/qerror.h > @@ -66,6 +66,9 @@ QError *qobject_to_qerror(const QObject *obj); > #define QERR_COMMAND_NOT_FOUND \ > "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }" > > +#define QERR_COMMAND_DISABLED \ > + "{ 'class': 'CommandDisabled', 'data': { 'name': %s } }" > + > #define QERR_DEVICE_ENCRYPTED \ > "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }" >
diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h index f1c26e4..3cf1781 100644 --- a/qapi/qmp-core.h +++ b/qapi/qmp-core.h @@ -31,11 +31,14 @@ typedef struct QmpCommand QmpCommandType type; QmpCommandFunc *fn; QTAILQ_ENTRY(QmpCommand) node; + bool enabled; } QmpCommand; void qmp_register_command(const char *name, QmpCommandFunc *fn); QmpCommand *qmp_find_command(const char *name); QObject *qmp_dispatch(QObject *request); +void qmp_disable_command(const char *name); +char **qmp_get_command_list(void); #endif diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 5584693..43f640a 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -79,6 +79,10 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp) error_set(errp, QERR_COMMAND_NOT_FOUND, command); return NULL; } + if (!cmd->enabled) { + error_set(errp, QERR_COMMAND_DISABLED, command); + return NULL; + } if (!qdict_haskey(dict, "arguments")) { args = qdict_new(); diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c index 5ff99cf..abafa34 100644 --- a/qapi/qmp-registry.c +++ b/qapi/qmp-registry.c @@ -14,7 +14,7 @@ #include "qapi/qmp-core.h" -static QTAILQ_HEAD(, QmpCommand) qmp_commands = +static QTAILQ_HEAD(QmpCommandList, QmpCommand) qmp_commands = QTAILQ_HEAD_INITIALIZER(qmp_commands); void qmp_register_command(const char *name, QmpCommandFunc *fn) @@ -24,17 +24,50 @@ void qmp_register_command(const char *name, QmpCommandFunc *fn) cmd->name = name; cmd->type = QCT_NORMAL; cmd->fn = fn; + cmd->enabled = true; QTAILQ_INSERT_TAIL(&qmp_commands, cmd, node); } QmpCommand *qmp_find_command(const char *name) { - QmpCommand *i; + QmpCommand *cmd; - QTAILQ_FOREACH(i, &qmp_commands, node) { - if (strcmp(i->name, name) == 0) { - return i; + QTAILQ_FOREACH(cmd, &qmp_commands, node) { + if (strcmp(cmd->name, name) == 0) { + return cmd; } } return NULL; } + +void qmp_disable_command(const char *name) +{ + QmpCommand *cmd; + + QTAILQ_FOREACH(cmd, &qmp_commands, node) { + if (strcmp(cmd->name, name) == 0) { + cmd->enabled = false; + return; + } + } +} + +char **qmp_get_command_list(void) +{ + QmpCommand *cmd; + int count = 1; + char **list_head, **list; + + QTAILQ_FOREACH(cmd, &qmp_commands, node) { + count++; + } + + list_head = list = g_malloc0(count * sizeof(char *)); + + QTAILQ_FOREACH(cmd, &qmp_commands, node) { + *list = strdup(cmd->name); + list++; + } + + return list_head; +} diff --git a/qemu-ga.c b/qemu-ga.c index 4932013..200bb15 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -27,6 +27,7 @@ #include "signal.h" #include "qerror.h" #include "error_int.h" +#include "qapi/qmp-core.h" #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0" #define QGA_PIDFILE_DEFAULT "/var/run/qemu-ga.pid" @@ -91,6 +92,8 @@ static void usage(const char *cmd) " -v, --verbose log extra debugging information\n" " -V, --version print version information and exit\n" " -d, --daemonize become a daemon\n" +" -b, --blacklist comma-seperated list of RPCs to disable (no spaces, \"?\"" +" to list available RPCs)\n" " -h, --help display this help and exit\n" "\n" "Report bugs to <mdroth@linux.vnet.ibm.com>\n" @@ -548,7 +551,7 @@ static void init_guest_agent(GAState *s) int main(int argc, char **argv) { - const char *sopt = "hVvdm:p:l:f:"; + const char *sopt = "hVvdm:p:l:f:b:"; const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT; const struct option lopt[] = { { "help", 0, NULL, 'h' }, @@ -559,13 +562,16 @@ int main(int argc, char **argv) { "method", 0, NULL, 'm' }, { "path", 0, NULL, 'p' }, { "daemonize", 0, NULL, 'd' }, + { "blacklist", 0, NULL, 'b' }, { NULL, 0, NULL, 0 } }; - int opt_ind = 0, ch, daemonize = 0; + int opt_ind = 0, ch, daemonize = 0, i, j, len; GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL; FILE *log_file = stderr; GAState *s; + module_call_init(MODULE_INIT_QAPI); + while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) { switch (ch) { case 'm': @@ -595,6 +601,32 @@ int main(int argc, char **argv) case 'd': daemonize = 1; break; + case 'b': { + char **list_head, **list; + if (*optarg == '?') { + list_head = list = qmp_get_command_list(); + while (*list != NULL) { + printf("%s\n", *list); + g_free(*list); + list++; + } + g_free(list_head); + return 0; + } + for (j = 0, i = 0, len = strlen(optarg); i < len; i++) { + if (optarg[i] == ',') { + optarg[i] = 0; + qmp_disable_command(&optarg[j]); + g_debug("disabling command: %s", &optarg[j]); + j = i + 1; + } + } + if (j < i) { + qmp_disable_command(&optarg[j]); + g_debug("disabling command: %s", &optarg[j]); + } + break; + } case 'h': usage(argv[0]); return 0; @@ -624,7 +656,6 @@ int main(int argc, char **argv) ga_command_state_init_all(s->command_state); ga_state = s; - module_call_init(MODULE_INIT_QAPI); init_guest_agent(ga_state); register_signal_handlers(); diff --git a/qerror.c b/qerror.c index 656efc2..a998d2f 100644 --- a/qerror.c +++ b/qerror.c @@ -65,6 +65,10 @@ static const QErrorStringTable qerror_table[] = { .desc = "The command %(name) has not been found", }, { + .error_fmt = QERR_COMMAND_DISABLED, + .desc = "The command %(name) has been disabled for this instance", + }, + { .error_fmt = QERR_DEVICE_ENCRYPTED, .desc = "Device '%(device)' is encrypted", }, diff --git a/qerror.h b/qerror.h index 161d654..0d3d4c5 100644 --- a/qerror.h +++ b/qerror.h @@ -66,6 +66,9 @@ QError *qobject_to_qerror(const QObject *obj); #define QERR_COMMAND_NOT_FOUND \ "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }" +#define QERR_COMMAND_DISABLED \ + "{ 'class': 'CommandDisabled', 'data': { 'name': %s } }" + #define QERR_DEVICE_ENCRYPTED \ "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }"
This adds a command-line option, -b/--blacklist, that accepts a comma-seperated list of RPCs to disable, or prints a list of available RPCs if passed "?". In consequence this also adds general blacklisting and RPC listing facilities to the new QMP dispatch/registry facilities, should the QMP monitor ever have a need for such a thing. Ideally, to avoid support/compatability issues in the future, blacklisting guest agent functionality will be the exceptional case, but we add the functionality here to handle guest administrators with specific requirements. Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> --- qapi/qmp-core.h | 3 +++ qapi/qmp-dispatch.c | 4 ++++ qapi/qmp-registry.c | 43 ++++++++++++++++++++++++++++++++++++++----- qemu-ga.c | 37 ++++++++++++++++++++++++++++++++++--- qerror.c | 4 ++++ qerror.h | 3 +++ 6 files changed, 86 insertions(+), 8 deletions(-)