[libvirt] [PATCH] util: Fix stack smashing in virNetDevGetFamilyId
After commit 8708ca01c0d libvirtd consistently aborts with "stack smashing detected" when nodedev driver is initialized. Apparently this is caused by nlmsg_parse trying to write more than CTRL_ATTR_MAX bytes in tb because it is told tb can accommodate CTRL_CMD_MAX bytes. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virnetdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 41a659732b..be8f4b7938 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3158,7 +3158,7 @@ virNetDevGetFamilyId(const char *family_name) struct nl_msg *nl_msg = NULL; struct nlmsghdr *resp = NULL; struct genlmsghdr* gmsgh = NULL; - struct nlattr *tb[CTRL_ATTR_MAX + 1] = {NULL, }; + struct nlattr *tb[CTRL_CMD_MAX + 1] = {NULL, }; unsigned int recvbuflen; uint32_t family_id = 0; -- 2.14.1
On 09/20/2017 06:48 AM, Jiri Denemark wrote:
After commit 8708ca01c0d libvirtd consistently aborts with "stack smashing detected" when nodedev driver is initialized.
Apparently this is caused by nlmsg_parse trying to write more than CTRL_ATTR_MAX bytes in tb because it is told tb can accommodate CTRL_CMD_MAX bytes.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virnetdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Including the original author on the reply... Is it possible that instead, the: if (nlmsg_parse(resp, sizeof(struct nlmsghdr), tb, CTRL_CMD_MAX, NULL) < 0) should use CTRL_ATTR_MAX? If I look at virNetDevSwitchdevFeature it has : struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {NULL, }; and uses: if (nlmsg_parse(resp, sizeof(struct genlmsghdr), tb, DEVLINK_ATTR_MAX, NULL) John
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 41a659732b..be8f4b7938 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3158,7 +3158,7 @@ virNetDevGetFamilyId(const char *family_name) struct nl_msg *nl_msg = NULL; struct nlmsghdr *resp = NULL; struct genlmsghdr* gmsgh = NULL; - struct nlattr *tb[CTRL_ATTR_MAX + 1] = {NULL, }; + struct nlattr *tb[CTRL_CMD_MAX + 1] = {NULL, }; unsigned int recvbuflen; uint32_t family_id = 0;
On Wed, Sep 20, 2017 at 12:48:01PM +0200, Jiri Denemark wrote:
After commit 8708ca01c0d libvirtd consistently aborts with "stack smashing detected" when nodedev driver is initialized.
Apparently this is caused by nlmsg_parse trying to write more than CTRL_ATTR_MAX bytes in tb because it is told tb can accommodate CTRL_CMD_MAX bytes.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virnetdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 41a659732b..be8f4b7938 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3158,7 +3158,7 @@ virNetDevGetFamilyId(const char *family_name) struct nl_msg *nl_msg = NULL; struct nlmsghdr *resp = NULL; struct genlmsghdr* gmsgh = NULL; - struct nlattr *tb[CTRL_ATTR_MAX + 1] = {NULL, }; + struct nlattr *tb[CTRL_CMD_MAX + 1] = {NULL, };
Shouldn't the correct fix be to instruct nlmsg_parse to accommodate up to CTRL_ATTR_MAX bytes instead? Judging from the nature of the enum in getlink.h it appears to me that that's the correct enum we want to use, instead of the CTR_CMD_ enum ?? I'm inclined to say ACK, but let me know what you think of the enum first. Erik
On 09/20/2017 07:41 AM, Erik Skultety wrote:
On Wed, Sep 20, 2017 at 12:48:01PM +0200, Jiri Denemark wrote:
After commit 8708ca01c0d libvirtd consistently aborts with "stack smashing detected" when nodedev driver is initialized.
Apparently this is caused by nlmsg_parse trying to write more than CTRL_ATTR_MAX bytes in tb because it is told tb can accommodate CTRL_CMD_MAX bytes.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virnetdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 41a659732b..be8f4b7938 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3158,7 +3158,7 @@ virNetDevGetFamilyId(const char *family_name) struct nl_msg *nl_msg = NULL; struct nlmsghdr *resp = NULL; struct genlmsghdr* gmsgh = NULL; - struct nlattr *tb[CTRL_ATTR_MAX + 1] = {NULL, }; + struct nlattr *tb[CTRL_CMD_MAX + 1] = {NULL, }; Shouldn't the correct fix be to instruct nlmsg_parse to accommodate up to CTRL_ATTR_MAX bytes instead? Judging from the nature of the enum in getlink.h it appears to me that that's the correct enum we want to use, instead of the CTR_CMD_ enum ??
That is correct. tb is an array of *nlattr, with one pointer for each possible attribute that could be encountered. The netlink response that's being parsed here will contain CTRL_ATTR_* attributes, and nlmsg_parse will 1) put a NULL in all the entries of tb (that's what's causing the problem), then 2) fill in each tb[CTRL_ATTR_BLAH] with a pointer to that attribute in the netlink response. So Erik's suggested fix is the correct fix.
I'm inclined to say ACK, but let me know what you think of the enum first.
Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
Erik Skultety -
Jiri Denemark -
John Ferlan -
Laine Stump