RE: [PATCH net] tipc: bound the device MTU accepted for L2 bearers

[Tung Quang Nguyen]

>Subject: [PATCH net] tipc: bound the device MTU accepted for L2 bearers
>
>struct tipc_bearer keeps the bearer MTU in a u32 while struct tipc_link keeps
>the link MTU in a u16.  tipc_enable_l2_media() and the NETDEV_CHANGEMTU
>notifier both do b->mtu = dev->mtu, tipc_link_create() assigns that into l->mtu,
>and tipc_link_set_queue_limits() divides by it:
>
>	int max_bulk = TIPC_MAX_PUBL / (l->mtu / ITEM_SIZE);
>
>ITEM_SIZE is 20, so a device MTU of 65536 truncates to 0 and the division
>faults.
>
>tipc_mtu_bad() is the only check on either assignment and tests a lower bound
>only.  The truncation defeats even that: a device MTU of 65556 passes it, yet
>the link comes out with l->mtu = 20, below the TIPC_MIN_BEARER_MTU the
>check exists to enforce, and tipc_link_mss() then evaluates 20 - INT_H_SIZE -
>EMSG_OVERHEAD as an int and stores it in the u32 tipc_link_entry::mtu.  Read
>back over TIPC_NL_LINK_GET, every device MTU above 65535 leaves the link
>with a wrong MTU: 20 at 65556,
>464 at 66000, 32768 at 98304.
>
>The bound added in commit 9f29cd8a8e79 ("tipc: fix u16 MTU truncation in
>media and bearer MTU validation") applies to TIPC_NLA_PROP_MTU in
>tipc_nl_prop_policy, which covers TIPC_NL_MEDIA_SET and
>TIPC_NL_BEARER_SET.  A device MTU never passes through that policy.
>
>dummy sets max_mtu to 0, and dev_validate_mtu() applies its ceiling only
>when max_mtu is non-zero, so the device accepts 65536; a macvlan inherits
>max_mtu from its lower device, and two macvlans in bridge mode on one
>dummy carry each other's TIPC discovery frames, so the bearer reaches
>tipc_node_check_dest() and
>tipc_link_create() with b->mtu = 65536.  TIPC_NL_BEARER_ENABLE is
>GENL_UNS_ADMIN_PERM, so an unprivileged user can do this in a user
>namespace of their own, on a kernel that has TIPC loaded.
>
>  Oops: divide error: 0000 [#1] SMP KASAN NOPTI
>  RIP: 0010:tipc_link_create (net/tipc/link.c:2531 net/tipc/link.c:520)
>  Call Trace:
>   <IRQ>
>   tipc_node_check_dest (net/tipc/node.c:1284)
>   tipc_disc_rcv (net/tipc/discover.c:252)
>   tipc_rcv (net/tipc/node.c:2134)
>   tipc_l2_rcv_msg (net/tipc/bearer.c:669)
>   __netif_receive_skb_one_core (net/core/dev.c:6216)
>   process_backlog (net/core/dev.c:6680)
>   __napi_poll (net/core/dev.c:7739)
>   net_rx_action (net/core/dev.c:7802)
>   handle_softirqs (kernel/softirq.c:622)
>   </IRQ>
>  Kernel panic - not syncing: Fatal exception in interrupt
>
>Give tipc_mtu_bad() the matching upper bound, so the limit sits next to the
>existing minimum and one check covers both assignments.  A device MTU of
>65535 is still accepted and the link still comes up, with the
>65532 that TIPC rounds it to.
>
>The u32 max_pkt fields became u16 mtu and advertised_mtu in commit
>ed193ece2649 ("tipc: simplify link mtu negotiation"), which also removed
>link_init_max_pkt(); that had clamped the bearer MTU to MAX_MSG_SIZE
>before it could reach this division.
>
>Fixes: ed193ece2649 ("tipc: simplify link mtu negotiation")
>Reported-by: Yunpeng Tian <shionthanatos@xxxxxxxxx>
>Reported-by: Gongming Wang <gmwgg05@xxxxxxxxx>
>Reported-by: Mingda Zhang <npczmd@xxxxxx>
>Reported-by: Xiaoyu Zhou <xiao-yu.zhou@xxxxxxxxxxxxxxxx>
>Reported-by: Qinrun Dai <jupmouse@xxxxxxxxx>
>Cc: stable@xxxxxxxxxxxxxxx # v4.1
>Signed-off-by: Yunpeng Tian <shionthanatos@xxxxxxxxx>

Next time:
1. Please use correct "Reported-by". Five reporters for a small issue looks weird.
2. Person in Signed-off-by should not be in Reported-by.

Reviewed-by: Tung Nguyen <tung.quang.nguyen@xxxxxxxx>