12:20 p.m.
While I'd love to give more context, I can't - I've found these on a
stale branch. However, they are still valid and worth of merging.
Michal Prívozník (5):
qemu: Use virStrcpy in qemuNodeGetSecurityModel()
qemu: Obtain @caps only after ACL check in qemuNodeGetSecurityModel
qemu: Fix retval if ACL check fails in qemuNodeGetSecurityModel
domain_conf: Parse full length of some <seclabel/> attributes
use more virStrcpy() and virStrcpyStatic()
src/conf/domain_conf.c | 6 ++----
src/libvirt-lxc.c | 5 ++---
src/qemu/qemu_driver.c | 23 +++++++++--------------
src/remote/remote_driver.c | 12 ++++--------
src/security/security_selinux.c | 3 +--
5 files changed, 18 insertions(+), 31 deletions(-)
--
2.26.2
12:20 p.m.
New subject: [PATCH 1/5] qemu: Use virStrcpy in qemuNodeGetSecurityModel()
The code we have there to copy seclabel model or doi can be
replaced by virStrcpy() calls which do exactly the same checks.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a376824854..a9e8f660c7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5930,7 +5930,6 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn,
virSecurityModelPtr secmodel)
{
virQEMUDriverPtr driver = conn->privateData;
- char *p;
g_autoptr(virCaps) caps = NULL;
memset(secmodel, 0, sizeof(*secmodel));
@@ -5946,23 +5945,21 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn,
caps->host.secModels[0].model == NULL)
return 0;
- p = caps->host.secModels[0].model;
- if (strlen(p) >= VIR_SECURITY_MODEL_BUFLEN-1) {
+ if (virStrcpy(secmodel->model, caps->host.secModels[0].model,
+ VIR_SECURITY_MODEL_BUFLEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("security model string exceeds max %d bytes"),
- VIR_SECURITY_MODEL_BUFLEN-1);
+ VIR_SECURITY_MODEL_BUFLEN - 1);
return -1;
}
- strcpy(secmodel->model, p);
- p = caps->host.secModels[0].doi;
- if (strlen(p) >= VIR_SECURITY_DOI_BUFLEN-1) {
+ if (virStrcpy(secmodel->doi, caps->host.secModels[0].doi,
+ VIR_SECURITY_DOI_BUFLEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("security DOI string exceeds max %d bytes"),
- VIR_SECURITY_DOI_BUFLEN-1);
+ VIR_SECURITY_DOI_BUFLEN - 1);
return -1;
}
- strcpy(secmodel->doi, p);
return 0;
}
--
2.26.2
12:20 p.m.
New subject: [PATCH 2/5] qemu: Obtain @caps only after ACL check in qemuNodeGetSecurityModel
Even though we are getting driver capabilities with
refresh=false (so that it is not expensive), we still should do
ACL check first because there is no point in bothering with the
capabilities if caller doesn't have permissions to call the API.
Also, this way the comment makes more sense.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a9e8f660c7..96ec84bd1c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5934,14 +5934,12 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn,
memset(secmodel, 0, sizeof(*secmodel));
- if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
- return 0;
-
if (virNodeGetSecurityModelEnsureACL(conn) < 0)
return 0;
/* We treat no driver as success, but simply return no data in *secmodel */
- if (caps->host.nsecModels == 0 ||
+ if (!(caps = virQEMUDriverGetCapabilities(driver, false)) ||
+ caps->host.nsecModels == 0 ||
caps->host.secModels[0].model == NULL)
return 0;
--
2.26.2
12:20 p.m.
New subject: [PATCH 3/5] qemu: Fix retval if ACL check fails in qemuNodeGetSecurityModel
While previously we returned 0 this is not correct. We have to
return a negative value to indicate error.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 96ec84bd1c..88324945ef 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5935,7 +5935,7 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn,
memset(secmodel, 0, sizeof(*secmodel));
if (virNodeGetSecurityModelEnsureACL(conn) < 0)
- return 0;
+ return -1;
/* We treat no driver as success, but simply return no data in *secmodel */
if (!(caps = virQEMUDriverGetCapabilities(driver, false)) ||
--
2.26.2
12:20 p.m.
New subject: [PATCH 4/5] domain_conf: Parse full length of some <seclabel/> attributes
In virSecurityLabelDefParseXML() we are parsing the <seclabel/>
element among with its attributes. Some of the attributes are
limited in length (because of virNodeGetSecurityModel()), however
some are not. And for the latter ones we don't need to use
virXMLPropStringLimit() to parse them. Moreover, using
VIR_SECURITY_LABEL_BUFLEN as the limit is wrong - we are not
storing the parsed strings into a static buffer of that size
rather than checking if the string passes string -> enum
conversion.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 384710da40..5a8947eeec 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7713,8 +7713,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
/* set default value */
seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
- p = virXMLPropStringLimit(ctxt->node, "type",
- VIR_SECURITY_LABEL_BUFLEN - 1);
+ p = virXMLPropString(ctxt->node, "type");
if (p) {
seclabel->type = virDomainSeclabelTypeFromString(p);
if (seclabel->type <= 0) {
@@ -7729,8 +7728,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
seclabel->relabel = false;
VIR_FREE(p);
- p = virXMLPropStringLimit(ctxt->node, "relabel",
- VIR_SECURITY_LABEL_BUFLEN-1);
+ p = virXMLPropString(ctxt->node, "relabel");
if (p) {
if (virStringParseYesNo(p, &seclabel->relabel) < 0) {
virReportError(VIR_ERR_XML_ERROR,
--
2.26.2
12:20 p.m.
New subject: [PATCH 5/5] use more virStrcpy() and virStrcpyStatic()
There are a few places where we open code virStrcpy() or
virStrcpyStatic(). Call respective functions instead.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt-lxc.c | 5 ++---
src/remote/remote_driver.c | 12 ++++--------
src/security/security_selinux.c | 3 +--
3 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c
index f6391214be..2a271b74f0 100644
--- a/src/libvirt-lxc.c
+++ b/src/libvirt-lxc.c
@@ -35,6 +35,7 @@
# include <sys/apparmor.h>
#endif
#include "vircgroup.h"
+#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -213,7 +214,7 @@ virDomainLxcEnterSecurityLabel(virSecurityModelPtr model,
goto error;
}
- if (strlen((char *) ctx) >= VIR_SECURITY_LABEL_BUFLEN) {
+ if (virStrcpy(oldlabel->label, ctx, VIR_SECURITY_LABEL_BUFLEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("security label exceeds "
"maximum length: %d"),
@@ -221,8 +222,6 @@ virDomainLxcEnterSecurityLabel(virSecurityModelPtr model,
freecon(ctx);
goto error;
}
-
- strcpy(oldlabel->label, (char *) ctx);
freecon(ctx);
if ((oldlabel->enforcing = security_getenforce()) < 0) {
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index b0af3ee88e..1b784e61c7 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -2328,12 +2328,11 @@ remoteDomainGetSecurityLabel(virDomainPtr domain,
virSecurityLabelPtr seclabel)
}
if (ret.label.label_val != NULL) {
- if (strlen(ret.label.label_val) >= sizeof(seclabel->label)) {
+ if (virStrcpyStatic(seclabel->label, ret.label.label_val) < 0) {
virReportError(VIR_ERR_RPC, _("security label exceeds maximum:
%zu"),
sizeof(seclabel->label) - 1);
goto cleanup;
}
- strcpy(seclabel->label, ret.label.label_val);
seclabel->enforcing = ret.enforcing;
}
@@ -2372,13 +2371,12 @@ remoteDomainGetSecurityLabelList(virDomainPtr domain,
virSecurityLabelPtr* secla
for (i = 0; i < ret.labels.labels_len; i++) {
remote_domain_get_security_label_ret *cur = &ret.labels.labels_val[i];
if (cur->label.label_val != NULL) {
- if (strlen(cur->label.label_val) >= sizeof((*seclabels)->label)) {
+ if (virStrcpyStatic((*seclabels)[i].label, cur->label.label_val) < 0)
{
virReportError(VIR_ERR_RPC, _("security label exceeds maximum:
%zd"),
sizeof((*seclabels)->label) - 1);
VIR_FREE(*seclabels);
goto cleanup;
}
- strcpy((*seclabels)[i].label, cur->label.label_val);
(*seclabels)[i].enforcing = cur->enforcing;
}
}
@@ -2444,21 +2442,19 @@ remoteNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr
secmodel)
}
if (ret.model.model_val != NULL) {
- if (strlen(ret.model.model_val) >= sizeof(secmodel->model)) {
+ if (virStrcpyStatic(secmodel->model, ret.model.model_val) < 0) {
virReportError(VIR_ERR_RPC, _("security model exceeds maximum:
%zu"),
sizeof(secmodel->model) - 1);
goto cleanup;
}
- strcpy(secmodel->model, ret.model.model_val);
}
if (ret.doi.doi_val != NULL) {
- if (strlen(ret.doi.doi_val) >= sizeof(secmodel->doi)) {
+ if (virStrcpyStatic(secmodel->doi, ret.doi.doi_val) < 0) {
virReportError(VIR_ERR_RPC, _("security doi exceeds maximum:
%zu"),
sizeof(secmodel->doi) - 1);
goto cleanup;
}
- strcpy(secmodel->doi, ret.doi.doi_val);
}
rv = 0;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index e9cd95916e..2fc6ef2616 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1209,7 +1209,7 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
return -1;
}
- if (strlen((char *)ctx) >= VIR_SECURITY_LABEL_BUFLEN) {
+ if (virStrcpy(sec->label, ctx, VIR_SECURITY_LABEL_BUFLEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("security label exceeds "
"maximum length: %d"),
@@ -1218,7 +1218,6 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
return -1;
}
- strcpy(sec->label, (char *)ctx);
freecon(ctx);
VIR_DEBUG("label=%s", sec->label);
--
2.26.2
1:08 p.m.
On a Monday in 2021, Michal Privoznik wrote:
While I'd love to give more context, I can't - I've found
these on a
stale branch. However, they are still valid and worth of merging.
Michal Prívozník (5):
qemu: Use virStrcpy in qemuNodeGetSecurityModel()
qemu: Obtain @caps only after ACL check in qemuNodeGetSecurityModel
qemu: Fix retval if ACL check fails in qemuNodeGetSecurityModel
domain_conf: Parse full length of some <seclabel/> attributes
use more virStrcpy() and virStrcpyStatic()
src/conf/domain_conf.c | 6 ++----
src/libvirt-lxc.c | 5 ++---
src/qemu/qemu_driver.c | 23 +++++++++--------------
src/remote/remote_driver.c | 12 ++++--------
src/security/security_selinux.c | 3 +--
5 files changed, 18 insertions(+), 31 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1420
days inactive
1420
days old
6 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik