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

[Laine Stump]

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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>