[B.A.T.M.A.N.] question about min_t() casting in batadv_hardif_min_mtu()

[B.A.T.M.A.N.] question about min_t() casting in batadv_hardif_min_mtu()

[Dan Carpenter]

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.

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.

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

   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

Re: [B.A.T.M.A.N.] question about min_t() casting in batadv_hardif_min_mtu()

[Antonio Quartulli]

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] question about min_t() casting in batadv_hardif_min_mtu()

[Dan Carpenter]

On Thu, Jan 30, 2014 at 02:59:07PM +0100, Antonio Quartulli wrote:
> 
> But why don't dev_set_mtu() check for the argument to be larger than
> 68bytes? Isn't that a general requirement?

I'm not a networking person.  It sounds reasonable to me that we could
use add a better minimum check there but why is it 68 bytes?

regards,
dan carpenter