* [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses
@ 2018-04-13 18:16 Sven Eckelmann
2018-04-14 2:34 ` Antonio Quartulli
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Sven Eckelmann @ 2018-04-13 18:16 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Sven Eckelmann, Andre Kasper
The IP translation layer is using the neighbor table of the kernel to get
the unicast link layer (mac) address for IP(v4|v6) addresses. The kernel
can not only return unicast mac addresses to such an RTM_GETNEIGH request
but also zero mac address. Such an address must be considered invalid
because the global translation table may not only contain a unique client
mac address entry for it. The translation from client mac to originator
will therefore most likely return an unexpected originator.
Dropping these kind of (bogus) results avoids confusions while using things
like batctl's ping or traceroute.
Reported-by: Andre Kasper <Andre.Kasper@gmx.de>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Cc: Andre Kasper <Andre.Kasper@gmx.de>
See https://www.open-mesh.org/issues/353
---
functions.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/functions.c b/functions.c
index cd92b60..3c340a2 100644
--- a/functions.c
+++ b/functions.c
@@ -571,6 +571,19 @@ static struct nla_policy neigh_policy[NDA_MAX+1] = {
[NDA_PROBES] = { .type = NLA_U32 },
};
+static bool ether_addr_valid(const uint8_t *addr)
+{
+ /* no multicast address */
+ if (addr[0] & 0x01)
+ return false;
+
+ /* no zero address */
+ if ((addr[0] | addr[1] | addr[2] | addr[3] | addr[4] | addr[5]) == 0)
+ return false;
+
+ return true;
+}
+
static int resolve_mac_from_parse(struct nl_msg *msg, void *arg)
{
struct nlattr *tb[NDA_MAX + 1];
@@ -616,6 +629,9 @@ static int resolve_mac_from_parse(struct nl_msg *msg, void *arg)
mac = nla_data(tb[NDA_LLADDR]);
l3addr = nla_data(tb[NDA_DST]);
+ if (!ether_addr_valid(mac))
+ goto err;
+
if (memcmp(nl_arg->l3addr, l3addr, l3_len) == 0) {
memcpy(nl_arg->mac_result, mac, ETH_ALEN);
nl_arg->found = 1;
--
2.17.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses
2018-04-13 18:16 [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses Sven Eckelmann
@ 2018-04-14 2:34 ` Antonio Quartulli
2018-04-14 7:10 ` Sven Eckelmann
2018-04-14 9:36 ` Antonio Quartulli
2018-04-17 8:35 ` Sven Eckelmann
2 siblings, 1 reply; 8+ messages in thread
From: Antonio Quartulli @ 2018-04-14 2:34 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking,
Sven Eckelmann
Cc: Andre Kasper
[-- Attachment #1.1: Type: text/plain, Size: 875 bytes --]
On 14/04/18 02:16, Sven Eckelmann wrote:
> The IP translation layer is using the neighbor table of the kernel to get
> the unicast link layer (mac) address for IP(v4|v6) addresses. The kernel
> can not only return unicast mac addresses to such an RTM_GETNEIGH request
> but also zero mac address. Such an address must be considered invalid
> because the global translation table may not only contain a unique client
> mac address entry for it. The translation from client mac to originator
> will therefore most likely return an unexpected originator.
>
We already handle the case of multiple originators handling the same MAC
address, no? In that case I think we pick the "best" originator.
This case sounds more like a validity check because "a zero MAC should
not be in the translation table", or am I wrong?
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses
2018-04-14 2:34 ` Antonio Quartulli
@ 2018-04-14 7:10 ` Sven Eckelmann
2018-04-14 8:11 ` Antonio Quartulli
0 siblings, 1 reply; 8+ messages in thread
From: Sven Eckelmann @ 2018-04-14 7:10 UTC (permalink / raw)
To: Antonio Quartulli
Cc: The list for a Better Approach To Mobile Ad-hoc Networking,
Andre Kasper, linus.luessing
[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]
On Samstag, 14. April 2018 04:34:42 CEST Antonio Quartulli wrote:
>
> On 14/04/18 02:16, Sven Eckelmann wrote:
[...]
> We already handle the case of multiple originators handling the same MAC
> address, no? In that case I think we pick the "best" originator.
Yes, but this doesn't make a lot of sense for multicast and zero mac
addresses. The translate layer of batctl is usually used to ping/traceroute to
some originator. But multicast and zero mac addresses don't represent a
"client" which can be used to identify some originator. So it doesn't seem to
make sense to allow them here.
Or even without the ping/traceroute stuff, the concept of calling `batctl
translate` should give you an answer which you can understand. So it should
tell you that batman-adv is very likely to transmit a unicast packet with this
destination address to this originator. But this cannot work for multicast
destination addresses because multiple answer should be given here - which is
out of scope for this command. Which reminds me that I should propose a second
patch which checks whether the input for translate_mac is "valid" before
trying to translate it.
> This case sounds more like a validity check because "a zero MAC should
> not be in the translation table", or am I wrong?
Partially, yes. I personally don't care (at the moment) whether there is a
zero mac address in the translation table. The current translation table code
(batadv_tt_local_add) doesn't check whether there is a zero mac address
(is_zero_ether_addr). But Linus had some ideas when zero mac addresses can be
useful - maybe he tell us whether it makes sense/problems to have them in the
translation table.
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses
2018-04-14 7:10 ` Sven Eckelmann
@ 2018-04-14 8:11 ` Antonio Quartulli
2018-04-14 9:20 ` Sven Eckelmann
0 siblings, 1 reply; 8+ messages in thread
From: Antonio Quartulli @ 2018-04-14 8:11 UTC (permalink / raw)
To: Sven Eckelmann
Cc: The list for a Better Approach To Mobile Ad-hoc Networking,
Andre Kasper, linus.luessing
[-- Attachment #1.1: Type: text/plain, Size: 2383 bytes --]
On 14/04/18 15:10, Sven Eckelmann wrote:
> On Samstag, 14. April 2018 04:34:42 CEST Antonio Quartulli wrote:
>>
>> On 14/04/18 02:16, Sven Eckelmann wrote:
> [...]
>> We already handle the case of multiple originators handling the same MAC
>> address, no? In that case I think we pick the "best" originator.
>
> Yes, but this doesn't make a lot of sense for multicast and zero mac
> addresses. The translate layer of batctl is usually used to ping/traceroute to
> some originator. But multicast and zero mac addresses don't represent a
> "client" which can be used to identify some originator. So it doesn't seem to
> make sense to allow them here.
Right.
>
> Or even without the ping/traceroute stuff, the concept of calling `batctl
> translate` should give you an answer which you can understand. So it should
> tell you that batman-adv is very likely to transmit a unicast packet with this
> destination address to this originator. But this cannot work for multicast
> destination addresses because multiple answer should be given here - which is
> out of scope for this command. Which reminds me that I should propose a second
> patch which checks whether the input for translate_mac is "valid" before
> trying to translate it.
>
>> This case sounds more like a validity check because "a zero MAC should
>> not be in the translation table", or am I wrong?
>
> Partially, yes. I personally don't care (at the moment) whether there is a
> zero mac address in the translation table. The current translation table code
> (batadv_tt_local_add) doesn't check whether there is a zero mac address
> (is_zero_ether_addr). But Linus had some ideas when zero mac addresses can be
> useful - maybe he tell us whether it makes sense/problems to have them in the
> translation table.
Yeah, I agree that zero "may appear" in the table and therefore we have
to "check" what we are getting back and whether it makes sense for us in
this context.
Actually my comment was not about changing your approach, but just
making it more explicit in the commit and in the error message.
An error message like like "returned invalid all-zero mac address" (or
"multicast address") might help to distinguish similar "ambiguities" in
the future. No?
Cheers,
>
> Kind regards,
> Sven
>
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses
2018-04-14 8:11 ` Antonio Quartulli
@ 2018-04-14 9:20 ` Sven Eckelmann
2018-04-14 9:35 ` Antonio Quartulli
0 siblings, 1 reply; 8+ messages in thread
From: Sven Eckelmann @ 2018-04-14 9:20 UTC (permalink / raw)
To: Antonio Quartulli
Cc: The list for a Better Approach To Mobile Ad-hoc Networking,
Andre Kasper, linus.luessing
[-- Attachment #1: Type: text/plain, Size: 756 bytes --]
On Samstag, 14. April 2018 10:11:28 CEST Antonio Quartulli wrote:
> An error message like like "returned invalid all-zero mac address" (or
> "multicast address") might help to distinguish similar "ambiguities" in
> the future. No?
The current interface for the translation is "give me a string and I return
NULL or a mac address". The resolving of the IPs for hostnames and the check
of the neighbor table are done on an "I take what I get first" approach. Your
suggestion would involve a change of this interface and parsing of additional
information and tracking of states to make sure that the "best" result is
returned (or a special error state). This is nothing which I will implement
now.
Should I drop the patch for now?
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses
2018-04-14 9:20 ` Sven Eckelmann
@ 2018-04-14 9:35 ` Antonio Quartulli
0 siblings, 0 replies; 8+ messages in thread
From: Antonio Quartulli @ 2018-04-14 9:35 UTC (permalink / raw)
To: Sven Eckelmann
Cc: The list for a Better Approach To Mobile Ad-hoc Networking,
Andre Kasper, linus.luessing
[-- Attachment #1.1: Type: text/plain, Size: 1127 bytes --]
On 14/04/18 17:20, Sven Eckelmann wrote:
> On Samstag, 14. April 2018 10:11:28 CEST Antonio Quartulli wrote:
>> An error message like like "returned invalid all-zero mac address" (or
>> "multicast address") might help to distinguish similar "ambiguities" in
>> the future. No?
>
> The current interface for the translation is "give me a string and I return
> NULL or a mac address". The resolving of the IPs for hostnames and the check
> of the neighbor table are done on an "I take what I get first" approach. Your
> suggestion would involve a change of this interface and parsing of additional
> information and tracking of states to make sure that the "best" result is
> returned (or a special error state). This is nothing which I will implement
> now.
Oh ok, I went through the code and now I better understand what you meant.
>
> Should I drop the patch for now?
>
I'd suggest to still consider it for merging.
Even if we can't be specific about the error, it is still better to stop
the translation rather than generating unexpected results.
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses
2018-04-13 18:16 [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses Sven Eckelmann
2018-04-14 2:34 ` Antonio Quartulli
@ 2018-04-14 9:36 ` Antonio Quartulli
2018-04-17 8:35 ` Sven Eckelmann
2 siblings, 0 replies; 8+ messages in thread
From: Antonio Quartulli @ 2018-04-14 9:36 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking,
Sven Eckelmann
Cc: Andre Kasper
[-- Attachment #1.1: Type: text/plain, Size: 872 bytes --]
On 14/04/18 02:16, Sven Eckelmann wrote:
> The IP translation layer is using the neighbor table of the kernel to get
> the unicast link layer (mac) address for IP(v4|v6) addresses. The kernel
> can not only return unicast mac addresses to such an RTM_GETNEIGH request
> but also zero mac address. Such an address must be considered invalid
> because the global translation table may not only contain a unique client
> mac address entry for it. The translation from client mac to originator
> will therefore most likely return an unexpected originator.
>
> Dropping these kind of (bogus) results avoids confusions while using things
> like batctl's ping or traceroute.
>
> Reported-by: Andre Kasper <Andre.Kasper@gmx.de>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
Acked-by: Antonio Quartulli <a@unstable.cc>
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses
2018-04-13 18:16 [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses Sven Eckelmann
2018-04-14 2:34 ` Antonio Quartulli
2018-04-14 9:36 ` Antonio Quartulli
@ 2018-04-17 8:35 ` Sven Eckelmann
2 siblings, 0 replies; 8+ messages in thread
From: Sven Eckelmann @ 2018-04-17 8:35 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Andre Kasper
[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]
On Freitag, 13. April 2018 20:16:18 CEST Sven Eckelmann wrote:
> The IP translation layer is using the neighbor table of the kernel to get
> the unicast link layer (mac) address for IP(v4|v6) addresses. The kernel
> can not only return unicast mac addresses to such an RTM_GETNEIGH request
> but also zero mac address. Such an address must be considered invalid
> because the global translation table may not only contain a unique client
> mac address entry for it. The translation from client mac to originator
> will therefore most likely return an unexpected originator.
>
> Dropping these kind of (bogus) results avoids confusions while using things
> like batctl's ping or traceroute.
>
> Reported-by: Andre Kasper <Andre.Kasper@gmx.de>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> Cc: Andre Kasper <Andre.Kasper@gmx.de>
>
> See https://www.open-mesh.org/issues/353
Applied in 9f22e3d0ed2a [1]. But the ticket is not marked as resolved but it
seems like the reporter wants a different behavior.
Kind regards,
Sven
[1] https://git.open-mesh.org/batctl.git/commit/9f22e3d0ed2a3d83479f3db3b570f49218024249
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-04-17 8:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-13 18:16 [B.A.T.M.A.N.] [PATCH] batctl: Validate translated mac addresses Sven Eckelmann
2018-04-14 2:34 ` Antonio Quartulli
2018-04-14 7:10 ` Sven Eckelmann
2018-04-14 8:11 ` Antonio Quartulli
2018-04-14 9:20 ` Sven Eckelmann
2018-04-14 9:35 ` Antonio Quartulli
2018-04-14 9:36 ` Antonio Quartulli
2018-04-17 8:35 ` Sven Eckelmann
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.