[B.A.T.M.A.N.] [PATCH 1/6] alfred: Reduce MAX_PAYLOAD by the size of the UDP header

[B.A.T.M.A.N.] [PATCH 1/6] alfred: Reduce MAX_PAYLOAD by the size of the UDP header

[Sven Eckelmann]

The len field in the UDP header is 16 bit long. It can therefore store a
value of up to (2 ** 16 - 1). This value is currently used for MAX_PAYLOAD.
But the UDP len field not only stores the payload length but also the udp
header itself. Thus the length of MAX_PAYLOAD has to be reduced by the
size of udphdr to make sure that the payload can still be sent.

Reported-by: Hans-Werner Hilse <hwhilse@gmail.com>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 alfred.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/alfred.h b/alfred.h
index 8fc8ab1..7c308e5 100644
--- a/alfred.h
+++ b/alfred.h
@@ -26,4 +26,5 @@
 #include <net/ethernet.h>
 #include <netinet/in.h>
+#include <netinet/udp.h>
 #include <stdint.h>
 #include <time.h>
@@ -138,5 +139,5 @@ struct globals {
 #define debugFree(ptr, num)	free(ptr)
 
-#define MAX_PAYLOAD ((1 << 16) - 1)
+#define MAX_PAYLOAD ((1 << 16) - 1 - sizeof(struct udphdr))
 
 extern const struct in6_addr in6addr_localmcast;
-- 
2.1.4

[B.A.T.M.A.N.] [PATCH 2/6] alfred: Make buffer size check before sending explicit

[Sven Eckelmann]

It is checked when data is send by checking if the data would fit inside
the outgoing UDP packet. But it is not checked if the data would fit after
the sending was done. This doesn't have to be true just from the
restrictions which can be seen in this function. So just check if the data
and its headers would now fit in outgoing buffer before copying the data to
the output buffer.

This is not a problem by itself because the data + header in the dataset
cannot be larger than (MAX_PAYLOAD - sizeof(struct alfred_push_data_v0)).

Reported-by: Hans-Werner Hilse <hwhilse@gmail.com>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 send.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/send.c b/send.c
index 8853970..5a92132 100644
--- a/send.c
+++ b/send.c
@@ -92,4 +92,9 @@ int push_data(struct globals *globals, struct interface *interface,
 		}
 
+		/* still too large? - should never happen */
+		if (total_length + dataset->data.header.length + sizeof(*data) >
+		    MAX_PAYLOAD - sizeof(*push))
+			continue;
+
 		data = (struct alfred_data *)
 		       (buf + sizeof(*push) + total_length);
-- 
2.1.4

Re: [B.A.T.M.A.N.] [PATCH 2/6] alfred: Make buffer size check before sending explicit

[Sven Eckelmann]

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

On Sunday 31 May 2015 13:35:56 Sven Eckelmann wrote:
> It is checked when data is send by checking if the data would fit inside
> the outgoing UDP packet. But it is not checked if the data would fit after
> the sending was done. This doesn't have to be true just from the
> restrictions which can be seen in this function. So just check if the data
> and its headers would now fit in outgoing buffer before copying the data to
> the output buffer.
> 
> This is not a problem by itself because the data + header in the dataset
> cannot be larger than (MAX_PAYLOAD - sizeof(struct alfred_push_data_v0)).

Alternative commit message:

The sending code is automatically transmitting a packet when the next data
block would not fit inside the outgoing, aggregated UDP packet. But the
code does not check whether the data would then fit inside the new,
complete empty push_data packet. It is currently no problem because alfred
has the restriction that a dataset never stores a buffer larger than
(MAX_PAYLOAD - sizeof(struct alfred_push_data_v0) - sizeof(struct alfred_data)).
Therefore, the length check for the empty push_data packet + dataset buffer
would never fail.

Nonetheless, make this check explicit to avoid problems when the receiving
code is changed or the sending code gets the ability to limit the size of
outgoing UDP packets.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[B.A.T.M.A.N.] [PATCH 3/6] alfred: Don't push data when nothing is available

[Sven Eckelmann]

The push data code currently sends packets with MAX_PAYLOAD. This allows
some checks to be dropped. For example, the check if data must be send out
is just a check if the current data still can be copied to the output
buffer. But this would also mean that data with a size larger than
(MAX_PAYLOAD - sizeof(struct alfred_push_data_v0)) may trigger an empty
packet.

This is not a problem by itself because currently the data + header in
the dataset cannot be larger than
(MAX_PAYLOAD - sizeof(struct alfred_push_data_v0)).

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 send.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/send.c b/send.c
index 5a92132..43c10f4 100644
--- a/send.c
+++ b/send.c
@@ -83,4 +83,8 @@ int push_data(struct globals *globals, struct interface *interface,
 		if (total_length + dataset->data.header.length + sizeof(*data) >
 		    MAX_PAYLOAD - sizeof(*push)) {
+			/* is there any data to send? */
+			if (total_length == 0)
+				continue;
+
 			tlv_length = total_length;
 			tlv_length += sizeof(*push) - sizeof(push->header);
-- 
2.1.4

[B.A.T.M.A.N.] [PATCH 4/6] alfred: Check buffer size before pushing data over unix socket

[Sven Eckelmann]

Check if the data and its headers would fit in outgoing buffer before
copying the data to the output buffer. This is not a problem by itself
because the data + header in the dataset cannot be larger than
(MAX_PAYLOAD - sizeof(struct alfred_push_data_v0)).

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 unix_sock.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/unix_sock.c b/unix_sock.c
index 2c862d5..29c934e 100644
--- a/unix_sock.c
+++ b/unix_sock.c
@@ -183,4 +183,9 @@ static int unix_sock_req_data_reply(struct globals *globals, int client_sock,
 			continue;
 
+		/* too large? - should never happen */
+		if (dataset->data.header.length + sizeof(*data) >
+		    MAX_PAYLOAD - sizeof(*push))
+			continue;
+
 		data = push->data;
 		memcpy(data, &dataset->data, sizeof(*data));
-- 
2.1.4

[B.A.T.M.A.N.] [PATCH 5/6] alfred: First check only push_data header length

[Sven Eckelmann]

The length in the TLV header for push_data is without the TLV header
itself. So the length check should instead only check for this size because
the other size checks will be done later.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 unix_sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/unix_sock.c b/unix_sock.c
index 29c934e..570c62c 100644
--- a/unix_sock.c
+++ b/unix_sock.c
@@ -109,5 +109,5 @@ static int unix_sock_add_data(struct globals *globals,
 	len = ntohs(push->header.length);
 
-	if (len < (int)(sizeof(*push) + sizeof(push->header)))
+	if (len < (int)(sizeof(*push) - sizeof(push->header)))
 		goto err;
 
-- 
2.1.4

[B.A.T.M.A.N.] [PATCH 6/6] alfred: Drop small push_data packets early

[Sven Eckelmann]

The push_data packets which are too small can be dropped earlier in the
transaction process. These cannot be parsed later when finishing the
transaction and thus it is unnecessary to first enqueue them to the
transaction list and allocate extra memory for the management structure.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 recv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/recv.c b/recv.c
index 16242bc..f459190 100644
--- a/recv.c
+++ b/recv.c
@@ -189,4 +189,6 @@ static int process_alfred_push_data(struct globals *globals,
 
 	len = ntohs(push->header.length);
+	if (len < (int)(sizeof(*push) - sizeof(push->header)))
+		goto err;
 
 	search.server_addr = mac;
-- 
2.1.4

Re: [B.A.T.M.A.N.] [PATCH 1/6] alfred: Reduce MAX_PAYLOAD by the size of the UDP header

[Simon Wunderlich]

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

On Sunday 31 May 2015 13:35:55 Sven Eckelmann wrote:
> The len field in the UDP header is 16 bit long. It can therefore store a
> value of up to (2 ** 16 - 1). This value is currently used for MAX_PAYLOAD.
> But the UDP len field not only stores the payload length but also the udp
> header itself. Thus the length of MAX_PAYLOAD has to be reduced by the
> size of udphdr to make sure that the payload can still be sent.
> 
> Reported-by: Hans-Werner Hilse <hwhilse@gmail.com>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Applied the whole series with the alternative wording for Patch 2.

Thanks!
    Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]