Re: [PATCH net-next 1/4] netlink: truncate overlength attribute list in nla_nest_end()

[Edwin Peer <edwin.peer@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]

On Sat, Jan 23, 2021 at 11:14 AM David Ahern <dsahern@gmail.com> wrote:

> > Marking truncated attributes, such that user space can determine
> > the precise attribute truncated, by means of an additional bit in
> > the nla_type was considered and rejected. The NLA_F_NESTED and
> > NLA_F_NET_BYTEORDER flags are supposed to be mutually exclusive.
> > So, in theory, the latter bit could have been redefined for nested
> > attributes in order to indicate truncation, but user space tools
> > (most notably iproute2) cannot be relied on to honor NLA_TYPE_MASK,
> > resulting in alteration of the perceived nla_type and subsequent
> > catastrophic failure.
>
> Did you look at using NETLINK_CB / netlink_skb_parms to keep a running
> length of nested attributes to avoid the need to trim?

I did not, but thinking about it now, I don't think that's necessarily
the way to go. We shouldn't be concerned about the cost of iterating
over the list and trimming the skb for what should be a rare exception
path. Ideally, we want to make sure at compile time (by having correct
code) that we don't ever exceed this limit at run time. Perhaps we
should investigate static analysis approaches to prove nla_len can't
be exceeded?

Tracking the outer nest length during nla_put() would provide for
convenient error indication at the precise location where things go
wrong, but that's a fair amount of housekeeping that isn't free of
complexity and run time cost either. Instead of rarely (if ever)
undoing work, we'll always do extra work that we hardly ever need.

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). If there are
other cases we don't know about today, they might turn from soft fails
into hard errors, breaking existing user space. Truncating the list is
the only non-obtrusive solution to any existing brokenness that is
guaranteed to not make things worse, but we can't know where we need
to do that apriori and would need to explicitly handle each case as
they come up.

Hard errors on nest overflow can only reliably work for new code. That
is, assuming it is tested to the extremes when it goes in, not after
user space comes to rely on the broken behavior.

Regards,
Edwin Peer

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4160 bytes --]