Re: [libvirt] [PATCH] util: Fix stack smashing in virNetDevGetFamilyId
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list