7:56 a.m.
The problem lies in how elements are appended into the transaction list -
instead of making a deep copy of a string we were doing a shallow copy only.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773
Erik Skultety (2):
security: DAC: fix the transaction model's list append
security: SELinux: fix the transaction model's list append
src/security/security_dac.c | 30 +++++++++++++++++++++---------
src/security/security_selinux.c | 30 +++++++++++++++++++++---------
2 files changed, 42 insertions(+), 18 deletions(-)
--
2.10.2
7:56 a.m.
New subject: [libvirt] [PATCH 1/2] security: DAC: fix the transaction model's list append
The problem is in the way how the list item is created prior to
appending it to the transaction list - the @path attribute is just a
shallow copy instead of deep copy of the hostdev device's path.
Unfortunately, the hostdev devices from which the @path is extracted, in
order to add them into the transaction list, are only temporary and
freed before the buildup of the qemu namespace, thus making the @path
attribute in the transaction list NULL, causing 'permission denied' or
'double free' or 'unknown cause' errors.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/security/security_dac.c | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index d457e6a..d7a2de4 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -71,7 +71,7 @@ struct _virSecurityDACCallbackData {
typedef struct _virSecurityDACChownItem virSecurityDACChownItem;
typedef virSecurityDACChownItem *virSecurityDACChownItemPtr;
struct _virSecurityDACChownItem {
- const char *path;
+ char *path;
const virStorageSource *src;
uid_t uid;
gid_t gid;
@@ -95,22 +95,32 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list,
uid_t uid,
gid_t gid)
{
- virSecurityDACChownItemPtr item;
+ int ret = -1;
+ char *tmp = NULL;
+ virSecurityDACChownItemPtr item = NULL;
if (VIR_ALLOC(item) < 0)
return -1;
- item->path = path;
+ if (VIR_STRDUP(tmp, path) < 0)
+ goto cleanup;
+
+ item->path = tmp;
item->src = src;
item->uid = uid;
item->gid = gid;
- if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) {
- VIR_FREE(item);
- return -1;
- }
+ if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0)
+ goto cleanup;
- return 0;
+ tmp = NULL;
+ item = NULL;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(tmp);
+ VIR_FREE(item);
+ return ret;
}
static void
@@ -122,8 +132,10 @@ virSecurityDACChownListFree(void *opaque)
if (!list)
return;
- for (i = 0; i < list->nItems; i++)
+ for (i = 0; i < list->nItems; i++) {
+ VIR_FREE(list->items[i]->path);
VIR_FREE(list->items[i]);
+ }
VIR_FREE(list);
}
--
2.10.2
8:17 a.m.
New subject: [libvirt] [PATCH 1/2] security: DAC: fix the transaction model's list append
On 01/17/2017 02:56 PM, Erik Skultety wrote:
The problem is in the way how the list item is created prior to
appending it to the transaction list - the @path attribute is just a
shallow copy instead of deep copy of the hostdev device's path.
Unfortunately, the hostdev devices from which the @path is extracted, in
order to add them into the transaction list, are only temporary and
freed before the buildup of the qemu namespace, thus making the @path
attribute in the transaction list NULL, causing 'permission denied' or
'double free' or 'unknown cause' errors.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/security/security_dac.c | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index d457e6a..d7a2de4 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -71,7 +71,7 @@ struct _virSecurityDACCallbackData {
typedef struct _virSecurityDACChownItem virSecurityDACChownItem;
typedef virSecurityDACChownItem *virSecurityDACChownItemPtr;
struct _virSecurityDACChownItem {
- const char *path;
+ char *path;
const virStorageSource *src;
uid_t uid;
gid_t gid;
@@ -95,22 +95,32 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list,
uid_t uid,
gid_t gid)
{
- virSecurityDACChownItemPtr item;
+ int ret = -1;
+ char *tmp = NULL;
+ virSecurityDACChownItemPtr item = NULL;
if (VIR_ALLOC(item) < 0)
return -1;
- item->path = path;
+ if (VIR_STRDUP(tmp, path) < 0)
+ goto cleanup;
+
+ item->path = tmp;
item->src = src;
item->uid = uid;
item->gid = gid;
- if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) {
- VIR_FREE(item);
- return -1;
- }
+ if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0)
+ goto cleanup;
- return 0;
+ tmp = NULL;
+ item = NULL;
This 'item = NULL' is not needed. VIR_APPEND_ELEMENT sets @item to NULL
upon successful return. But I agree that it is hard to spot.
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(tmp);
+ VIR_FREE(item);
+ return ret;
}
static void
@@ -122,8 +132,10 @@ virSecurityDACChownListFree(void *opaque)
if (!list)
return;
- for (i = 0; i < list->nItems; i++)
+ for (i = 0; i < list->nItems; i++) {
+ VIR_FREE(list->items[i]->path);
VIR_FREE(list->items[i]);
+ }
VIR_FREE(list);
}
ACK and safe for the freeze.
Michal
8:55 a.m.
New subject: [libvirt] [PATCH 1/2] security: DAC: fix the transaction model's list append
> - if (VIR_APPEND_ELEMENT(list->items, list->nItems,
item) < 0) {
> - VIR_FREE(item);
> - return -1;
> - }
> + if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0)
> + goto cleanup;
>
> - return 0;
> + tmp = NULL;
> + item = NULL;
This 'item = NULL' is not needed. VIR_APPEND_ELEMENT sets @item to NULL
upon successful return. But I agree that it is hard to spot.
Oh, missed that one, thanks for the suggestion.
Erik
7:56 a.m.
New subject: [libvirt] [PATCH 2/2] security: SELinux: fix the transaction model's list append
The problem is in the way how the list item is created prior to
appending it to the transaction list - the @path argument is just a
shallow copy instead of deep copy of the hostdev device's path.
Unfortunately, the hostdev devices from which the @path is extracted, in
order to add them into the transaction list, are only temporary and
freed before the buildup of the qemu namespace, thus making the @path
attribute in the transaction list NULL, causing 'permission denied' or
'double free' or 'unknown cause' errors.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/security/security_selinux.c | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index f229b51..c799056 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -81,7 +81,7 @@ struct _virSecuritySELinuxCallbackData {
typedef struct _virSecuritySELinuxContextItem virSecuritySELinuxContextItem;
typedef virSecuritySELinuxContextItem *virSecuritySELinuxContextItemPtr;
struct _virSecuritySELinuxContextItem {
- const char *path;
+ char *path;
const char *tcon;
bool optional;
};
@@ -111,21 +111,31 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr
list,
const char *tcon,
bool optional)
{
- virSecuritySELinuxContextItemPtr item;
+ int ret = -1;
+ char *tmp = NULL;
+ virSecuritySELinuxContextItemPtr item = NULL;
if (VIR_ALLOC(item) < 0)
return -1;
- item->path = path;
+ if (VIR_STRDUP(tmp, path) < 0)
+ goto cleanup;
+
+ item->path = tmp;
item->tcon = tcon;
item->optional = optional;
- if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) {
- VIR_FREE(item);
- return -1;
- }
+ if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0)
+ goto cleanup;
- return 0;
+ tmp = NULL;
+ item = NULL;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(tmp);
+ VIR_FREE(item);
+ return ret;
}
static void
@@ -137,8 +147,10 @@ virSecuritySELinuxContextListFree(void *opaque)
if (!list)
return;
- for (i = 0; i < list->nItems; i++)
+ for (i = 0; i < list->nItems; i++) {
+ VIR_FREE(list->items[i]->path);
VIR_FREE(list->items[i]);
+ }
VIR_FREE(list);
}
--
2.10.2
8:17 a.m.
New subject: [libvirt] [PATCH 2/2] security: SELinux: fix the transaction model's list append
On 01/17/2017 02:56 PM, Erik Skultety wrote:
The problem is in the way how the list item is created prior to
appending it to the transaction list - the @path argument is just a
shallow copy instead of deep copy of the hostdev device's path.
Unfortunately, the hostdev devices from which the @path is extracted, in
order to add them into the transaction list, are only temporary and
freed before the buildup of the qemu namespace, thus making the @path
attribute in the transaction list NULL, causing 'permission denied' or
'double free' or 'unknown cause' errors.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
src/security/security_selinux.c | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index f229b51..c799056 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -81,7 +81,7 @@ struct _virSecuritySELinuxCallbackData {
typedef struct _virSecuritySELinuxContextItem virSecuritySELinuxContextItem;
typedef virSecuritySELinuxContextItem *virSecuritySELinuxContextItemPtr;
struct _virSecuritySELinuxContextItem {
- const char *path;
+ char *path;
const char *tcon;
bool optional;
};
@@ -111,21 +111,31 @@
virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list,
const char *tcon,
bool optional)
{
- virSecuritySELinuxContextItemPtr item;
+ int ret = -1;
+ char *tmp = NULL;
+ virSecuritySELinuxContextItemPtr item = NULL;
if (VIR_ALLOC(item) < 0)
return -1;
- item->path = path;
+ if (VIR_STRDUP(tmp, path) < 0)
+ goto cleanup;
+
+ item->path = tmp;
item->tcon = tcon;
Unfortunately, while this was enough in the DAC driver, it is not enough
here. @tcon may be dynamically allocated just for this call:
virSecuritySELinuxRestoreFileLabel ->
virSecuritySELinuxSetFilecon ->
virSecuritySELinuxSetFileconHelper ->
virSecuritySELinuxTransactionAppend ->
virSecuritySELinuxContextListAppend
However, I guess fixing that is trivial. ACK if you do so and safe for
the freeze.
Michal
8:54 a.m.
New subject: [libvirt] [PATCH 2/2] security: SELinux: fix the transaction model's list append
> @@ -111,21 +111,31 @@
virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list,
> const char *tcon,
> bool optional)
> {
> - virSecuritySELinuxContextItemPtr item;
> + int ret = -1;
> + char *tmp = NULL;
> + virSecuritySELinuxContextItemPtr item = NULL;
>
> if (VIR_ALLOC(item) < 0)
> return -1;
>
> - item->path = path;
> + if (VIR_STRDUP(tmp, path) < 0)
> + goto cleanup;
> +
> + item->path = tmp;
> item->tcon = tcon;
Unfortunately, while this was enough in the DAC driver, it is not enough
here. @tcon may be dynamically allocated just for this call:
virSecuritySELinuxRestoreFileLabel ->
virSecuritySELinuxSetFilecon ->
virSecuritySELinuxSetFileconHelper ->
virSecuritySELinuxTransactionAppend ->
virSecuritySELinuxContextListAppend
However, I guess fixing that is trivial. ACK if you do so and safe for
the freeze.
Adjusted both patches according to your suggestions and pushed, thanks.
Erik
Michal
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2867
days inactive
2867
days old
6 comments
2 participants
participants (2)
-
Erik Skultety -
Michal Privoznik