Hello Dan,


On 30/01/14 13:52, Dan Carpenter wrote:
> Hello Batman devs,
> 
> I always worry when I see people do min_t(int, foo, bar) and there are
> a couple cases which concern me in batman.
> 
> Changing the mtu is allowed for people who have admin rights in their
> namespace.  In other words we check:
> 	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> as opposed to:
> 	if (!capable(net->user_ns, CAP_NET_ADMIN))
> So mtu has security implications.
> 

Thanks for sharing your concern and opinions

> net/batman-adv/hard-interface.c
>    240  int batadv_hardif_min_mtu(struct net_device *soft_iface)
>    241  {
>    242          struct batadv_priv *bat_priv = netdev_priv(soft_iface);
>    243          const struct batadv_hard_iface *hard_iface;
>    244          int min_mtu = ETH_DATA_LEN;
>    245  
>    246          rcu_read_lock();
>    247          list_for_each_entry_rcu(hard_iface, &batadv_hardif_list, list) {
>    248                  if ((hard_iface->if_status != BATADV_IF_ACTIVE) &&
>    249                      (hard_iface->if_status != BATADV_IF_TO_BE_ACTIVATED))
>    250                          continue;
>    251  
>    252                  if (hard_iface->soft_iface != soft_iface)
>    253                          continue;
>    254  
>    255                  min_mtu = min_t(int, hard_iface->net_dev->mtu, min_mtu);
>                                         ^^^
> Ok.  This is fine because dev_set_mtu() will not allow negative mtus.
> 
>    256          }
>    257          rcu_read_unlock();
>    258  
>    259          atomic_set(&bat_priv->packet_size_max, min_mtu);
>    260  
>    261          if (atomic_read(&bat_priv->fragmentation) == 0)
>    262                  goto out;
>    263  
>    264          /* with fragmentation enabled the maximum size of internally generated
>    265           * packets such as translation table exchanges or tvlv containers, etc
>    266           * has to be calculated
>    267           */
>    268          min_mtu = min_t(int, min_mtu, BATADV_FRAG_MAX_FRAG_SIZE);
>                                 ^^^
> Still fine.
> 
>    269          min_mtu -= sizeof(struct batadv_frag_packet);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> I worry that this could be negative.

Practically this "should" not happen because sizeof(struct
batadv_frag_packet) is 20 bytes and the smallest MTU over all the
interfaces that batman-adv is using should be not less than 68bytes (at
least this is what a sane Ethernet driver would allow).

Is there any Ethernet driver that allows the MTU to be smaller than 68?
Well, excluding all the broken ones :)

However this is a very good point! If we want to be on the safe side we
should check for min_mtu being always larger than a certain threshold
(e.g. 68 :-)).

> 
>    270          min_mtu *= BATADV_FRAG_MAX_FRAGMENTS;
>    271          atomic_set(&bat_priv->packet_size_max, min_mtu);
>                                                        ^^^^^^^
> Could a negative cause problems here?

Mh..I think this can generate an endless loop in
batadv_tt_local_resize_to_mtu()...

> 
>    272  
>    273          /* with fragmentation enabled we can fragment external packets easily */
>    274          min_mtu = min_t(int, min_mtu, ETH_DATA_LEN);
>                 ^^^^^^^
> min_mtu would still be negative here.
> 
>    275  
>    276  out:
>    277          return min_mtu - batadv_max_header_len();
>    278  }
> 
> regards,
> dan carpenter
> 

But why don't dev_set_mtu() check for the argument to be larger than
68bytes? Isn't that a general requirement?

Cheers,


-- 
Antonio Quartulli