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,