netdev - Re: [PATCH net-next 1/4] netlink: truncate overlength attribute list in nla_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]

Message-ID: <62a12b2c-c94e-8d89-0e75-f01dc6abbe92@gmail.com>
Date:   Mon, 25 Jan 2021 21:56:02 -0700
From:   David Ahern <dsahern@...il.com>
To:     Edwin Peer <edwin.peer@...adcom.com>
Cc:     netdev <netdev@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org>,
        Andrew Gospodarek <andrew.gospodarek@...adcom.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Michal Kubecek <mkubecek@...e.cz>
Subject: Re: [PATCH net-next 1/4] netlink: truncate overlength attribute list
 in nla_nest_end()

On 1/23/21 2:03 PM, Edwin Peer wrote:
> On Sat, Jan 23, 2021 at 12:42 PM Edwin Peer <edwin.peer@...adcom.com> wrote:
> 
>> Then, if nla_put() can detect nesting errors, there's the issue of
>> what to do in the case of errors. Case in point, the IFLA_VFINFO_LIST
>> scenario would now require explicit error handling in the generator
>> logic, because we can't fail hard at that point. We would need to be
>> sure we propagate all possible nesting errors up to a common location
>> (probably where the nest ends, which is where we're dealing with the
>> problem in this solution), set the truncated flag and carry on (for
>> the same net effect the trim in nla_nest_end() has).
> 
> Also, the unwind here turns out to be just as complicated, requiring a
> traversal from the start and a trim anyway, because the attribute that
> triggers the overflow may be deep inside the hierarchy. We can't
> simply truncate at this point. We should remove whole elements at the
> uppermost level, or risk having partially filled attribute trees
> rather than missing attributes at the level of the exceeded nest -
> which may be worse.
> 

I'm not a fan of the skb trim idea. I think it would be better to figure
out how to stop adding to the skb when an attr length is going to exceed
64kB. Not failing hard with an error (ip link sh needs to succeed), but
truncating the specific attribute of a message with a flag so userspace
knows it is short.