[libvirt] [PATCH] util: Fix stack smashing in virNetDevGetFamilyId

Jiri Denemark posted 1 patch 64 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/0e4dba7ec404a94a3eb0d63871f1c01233070340.1505904481.git.jdenemar@redhat.com
src/util/virnetdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

[libvirt] [PATCH] util: Fix stack smashing in virNetDevGetFamilyId

Posted by Jiri Denemark 64 weeks ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix stack smashing in virNetDevGetFamilyId

Posted by John Ferlan 64 weeks ago

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;
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix stack smashing in virNetDevGetFamilyId

Posted by Erik Skultety 64 weeks ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix stack smashing in virNetDevGetFamilyId

Posted by Laine Stump 64 weeks ago
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
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list