netdev - Re: [PATCH net-next] ethtool: strset: account for nesting in reply size

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]

Message-ID: <20210618233720.js4sk2xtgvf4ssn2@lion.mk-sys.cz>
Date:   Sat, 19 Jun 2021 01:37:20 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Saeed Mahameed <saeed@...nel.org>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Saeed Mahameed <saeedm@...dia.com>
Subject: Re: [PATCH net-next] ethtool: strset: account for nesting in reply
 size

On Fri, Jun 18, 2021 at 03:55:02PM -0700, Saeed Mahameed wrote:
> From: Saeed Mahameed <saeedm@...dia.com>
> 
> The cited patch revealed a bug in strset reply size where the
> calculation didn't include the 1st nla_nest_start(), a size of 4 Bytes in
> strset_fill_reply().
> 
> To fix the issue we account for the missing nla_nest 4Bytes by reporting
> them in strset_reply_size()
> 
> Before this patch issuing "ethtool -k" command will produce the
> following call trace:
[...]
> Fixes: 4d1fb7cde0cc ("ethtool: add a stricter length check")
> Fixes: 7c87e32d2e38 ("ethtool: count header size in reply size estimate")
> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> CC: Jakub Kicinski <kuba@...nel.org>
> CC: Michal Kubecek <mkubecek@...e.cz>

Actually, the history was the other way around: Jakub first fixed this
bug discovered by sysbot with commit e175aef90269 ("ethtool: strset: fix
message length calculation") in net tree and it inspired him to refine
the length check to catch such issues more likely. Unfortunately the fix
hasn't been merged into net-next yet which is why you saw the warning.
At least we know for sure now that the new version of the check works
much better than the old one.

> Note: I used nla_total_size(0); to report the missing bytes, i see in
> other places they use nla_total_size(sizeof(u32)). Since nla_nest uses a
> payload of 0, I prefer my version of nla_total_size(0); since it
> resembles what the nla_nest is actually doing. I might be wrong though
> :), comments ?

Out of the three fixes, personally I liked most the one which applied
nla_total_len() to calculated length of the nest contents as it IMHO
reflects the message structure best; but adding nla_total_size(0) also
provides the same result so either does the trick.

Michal

> ---
>  net/ethtool/strset.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
> index b3029fff715d..23d517a61e08 100644
> --- a/net/ethtool/strset.c
> +++ b/net/ethtool/strset.c
> @@ -349,8 +349,8 @@ static int strset_reply_size(const struct ethnl_req_info *req_base,
>  {
>  	const struct strset_req_info *req_info = STRSET_REQINFO(req_base);
>  	const struct strset_reply_data *data = STRSET_REPDATA(reply_base);
> +	int len = nla_total_size(0); /* account for nesting */
>  	unsigned int i;
> -	int len = 0;
>  	int ret;
>  
>  	for (i = 0; i < ETH_SS_COUNT; i++) {
> -- 
> 2.31.1
>