netdev - Re: [PATCH iproute2-next 3/3] treewide: Use addattr_nest()/addattr_nest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Apr 2018 15:57:37 -0700
From:   Vinicius Costa Gomes <vinicius.gomes@...el.com>
To:     Serhey Popovych <serhe.popovych@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH iproute2-next 3/3] treewide: Use addattr_nest()/addattr_nest_end() to handle nested attributes

Hi,

Serhey Popovych <serhe.popovych@...il.com> writes:

[...]

> diff --git a/tc/q_mqprio.c b/tc/q_mqprio.c
> index 89b4600..207d644 100644
> --- a/tc/q_mqprio.c
> +++ b/tc/q_mqprio.c
> @@ -173,8 +173,7 @@ static int mqprio_parse_opt(struct qdisc_util *qu, int argc,
>  		argc--; argv++;
>  	}
>  
> -	tail = NLMSG_TAIL(n);
> -	addattr_l(n, 1024, TCA_OPTIONS, &opt, sizeof(opt));
> +	tail = addattr_nest_compat(n, 1024, TCA_OPTIONS, &opt, sizeof(opt));
>  
>  	if (flags & TC_MQPRIO_F_MODE)
>  		addattr_l(n, 1024, TCA_MQPRIO_MODE,
> @@ -209,7 +208,7 @@ static int mqprio_parse_opt(struct qdisc_util *qu, int argc,
>  		addattr_nest_end(n, start);
>  	}
>  
> -	tail->rta_len = (void *)NLMSG_TAIL(n) - (void *)tail;
> +	addattr_nest_compat_end(n, tail);
>  
>  	return 0;
>  }

Sorry if I am too late, but this breaks mqprio, i.e. something like
this:

$ tc qdisc replace dev enp2s0 handle 100: parent root mqprio \
                   num_tc 3 map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \
                   queues 1@0 1@1 2@2 hw 0

that used to work, now doesn't.

This patch looks right, so I thought that it could be possible that mqprio
(in the kernel side) was making some wrong assumptions about the format
of the messages.

And after some investigation, what seems to be happening is something
like this (not too familiar with netlink protocol internals, I may be
missing something).

In the "wire", after this patch, the mqprio part of message may be
represented as:

/* The message format is [ len | type | payload ] */

[ S | 2 | <S bytes> ]
[ 0 | 2 | ]

Some notes:
 - S is the aligned value of sizeof(opt);
 - The value of TCA_OPTIONS is 2;

Before this patch, I think it was something like:

[ S | 2 | <S bytes> ]

The problem is that mqprio defines an internal type with the same value
as TCA_OPTIONS (2), and that finalizing (empty) is interpreted as the
"internal" field instead of indicating the end of TCA_OPTIONS, which
causes a size mismatch with 'mqprio_policy', causing the command to
create a mqprio qdisc to fail.

In short, I think that replacing the "open coded" version with
addattr_nest_compat() is not a functionally equivalent change.

Cheers,
--
Vinicius