* [B.A.T.M.A.N.] question about min_t() casting in batadv_hardif_min_mtu()
@ 2014-01-30 12:52 Dan Carpenter
2014-01-30 13:59 ` Antonio Quartulli
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2014-01-30 12:52 UTC (permalink / raw
To: sven, Marek Lindner; +Cc: b.a.t.m.a.n
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [B.A.T.M.A.N.] question about min_t() casting in batadv_hardif_min_mtu()
2014-01-30 12:52 [B.A.T.M.A.N.] question about min_t() casting in batadv_hardif_min_mtu() Dan Carpenter
@ 2014-01-30 13:59 ` Antonio Quartulli
2014-02-03 21:16 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Antonio Quartulli @ 2014-01-30 13:59 UTC (permalink / raw
To: dan.carpenter; +Cc: b.a.t.m.a.n
[-- 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 --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [B.A.T.M.A.N.] question about min_t() casting in batadv_hardif_min_mtu()
2014-01-30 13:59 ` Antonio Quartulli
@ 2014-02-03 21:16 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2014-02-03 21:16 UTC (permalink / raw
To: Antonio Quartulli; +Cc: b.a.t.m.a.n
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-02-03 21:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-30 12:52 [B.A.T.M.A.N.] question about min_t() casting in batadv_hardif_min_mtu() Dan Carpenter
2014-01-30 13:59 ` Antonio Quartulli
2014-02-03 21:16 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.