Re: BROKEN Re: [PATCH] netlink: 2-clause nla_ok()

[Johannes Berg]

> Maybe someone could vouch that other checks prevent
> this kind of situation from happening but not me.

No, now that you spell it out (and I see the patch) - this is
absolutely needed because nla_for_each_attr() [1] can be called on
arbitrary data coming from userspace in a message, e.g. by way
of nla_for_each_nested(). Even if it's not malformed, nla_ok() is the
only abort condition for that loop, so it would read at least one
nla_len after the real buffer without that condition.

johannes

[1] which seems to be the only significant user thereof

BROKEN Re: [PATCH] netlink: 2-clause nla_ok()

[Alexey Dobriyan]

David, please do

git revert 4f7df337fe79bba1e4c2d525525d63b5ba186bbd

I'm an idiot.

All rationale in the commit would be correct if reading "nla_len"
didn't require memory access. But it does.

return rem >= (int)sizeof(*nla) &&
   nla->nla_len >= sizeof(*nla) &&
   nla->nla_len <= remaining;

Those logical ands ensure that memory access is not done
if "rem" is small enough to even fetch ->nla_len.

Maybe someone could vouch that other checks prevent
this kind of situation from happening but not me.
How very embarrassing.

Signed-off-by: Alexey Dobriyan 

Re: [PATCH] netlink: 2-clause nla_ok()

[David Miller]

From: Alexey Dobriyan 
Date: Fri, 2 Dec 2016 03:59:06 +0300

> nla_ok() consists of 3 clauses:
> 
>   1) int rem >= (int)sizeof(struct nlattr)
> 
>   2) u16 nla_len >= sizeof(struct nlattr)
> 
>   3) u16 nla_len <= int rem
> 
> The statement is that clause (1) is redundant.
> 
> What it does is ensuring that "rem" is a positive number,
> so that in clause (3) positive number will be compared to positive number
> with no problems.
> 
> However, "u16" fully fits into "int" and integers do not change value
> when upcasting even to signed type. Negative integers will be rejected
> by clause (3) just fine. Small positive integers will be rejected
> by transitivity of comparison operator.
> 
> NOTE: all of the above DOES NOT apply to nlmsg_ok() where ->nlmsg_len is
> u32(!), so 3 clauses AND A CAST TO INT are necessary.
> 
> Obligatory space savings report: -1.6 KB
 ...
> Signed-off-by: Alexey Dobriyan 

Looks fine, applied to net-next, thanks.

[PATCH] netlink: 2-clause nla_ok()

[Alexey Dobriyan]

nla_ok() consists of 3 clauses:

1) int rem >= (int)sizeof(struct nlattr)

2) u16 nla_len >= sizeof(struct nlattr)

3) u16 nla_len <= int rem

The statement is that clause (1) is redundant.

What it does is ensuring that "rem" is a positive number,
so that in clause (3) positive number will be compared to positive number
with no problems.

However, "u16" fully fits into "int" and integers do not change value
when upcasting even to signed type. Negative integers will be rejected
by clause (3) just fine. Small positive integers will be rejected
by transitivity of comparison operator.

NOTE: all of the above DOES NOT apply to nlmsg_ok() where ->nlmsg_len is
u32(!), so 3 clauses AND A CAST TO INT are necessary.

Obligatory space savings report: -1.6 KB

$ ./scripts/bloat-o-meter ../vmlinux-000* ../vmlinux-001*
add/remove: 0/0 grow/shrink: 3/63 up/down: 35/-1692 (-1657)
function old new   delta
validate_scan_freqs  142 155 +13
tcf_em_tree_validate 867 879 +12
dcbnl_ieee_del   328 338 +10
netlbl_cipsov4_add_common.isra   218 215  -3
...
ovs_nla_put_actions  888 806 -82
netlbl_cipsov4_add_std  16481566 -82
nl80211_parse_sched_scan28892780-109
ip_tun_from_nlattr  30862945-141

Signed-off-by: Alexey Dobriyan 
---

 include/net/netlink.h |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -698,8 +698,7 @@ static inline int nla_len(const struct nlattr *nla)
  */
 static inline int nla_ok(const struct nlattr *nla, int remaining)
 {
-   return remaining >= (int) sizeof(*nla) &&
-  nla->nla_len >= sizeof(*nla) &&
+   return nla->nla_len >= sizeof(*nla) &&
   nla->nla_len <= remaining;
 }