firewire: net: IP-over-1394 link fragmentation fixes

The following patches
1/3 firewire: net: guard against rx buffer overflows 2/3 firewire: net: fix fragmented datagram_size off-by-one 3/3 firewire: net: max MTU off by one
fix a few long-standing bugs of the IP-over-1394 driver firewire-net related to reception and transmission of fragmented datagrams:

- RX: Missing validation of fragment offset and size makes the driver vulnerable to buffer overflows, potentially leading to remote¹ code execution. Reported by Eyal Itkin.

¹) The vulnerability cannot be triggered by malformed IP datagrams, but by malformed IEEE 1394 packets sent from other FireWire nodes to the 1394 broadcast channel or to firewire-net's unicast FIFO, or can be sent from the local node to the unicast FIFO by sufficiently privileged userland. I.e. an attack can only originate from somewhere on the FireWire bus, not from another network that is bridged to the FireWire bus.

- RX: Missing validation of unfragmented and fragmented datagrams for minimum packet size before looking at GASP header and encapsulation header.

- RX and TX: The datagram_size field of fragmented datagrams was read and written incorrectly; an offset of +/-1 needs to be applied. This prevents fragmented traffic from/to nodes which run OS X, Windows XP, or Linux' older eth1394 driver. (Traffic from Win XP would eventually be retried with smaller MTU and possibly succeed slowly despite the bug.)

Patch 1/3 is obviously urgent.

Patch 2/3 is a bit of a bother because while it fixes fragmented RX/TX with OS X, Win XP, and eth1394, it does disrupt fragmented RX/TX with Linux nodes which run an unfixed firewire-net.

Patch 3/3 will only apply in conjunction with changes that are queued up in the net-next git tree, hence this patch will wait until net-next was merged.

Patches 1+2/3 are already pushed out to linux1394.git "testing" and "for-next" branches, but I still like to get review comments before I send a pull request.

Stefan Richter
-======----- =-== ---=-
http://arcgraph.de/sr/

Date: Sat, 29 Oct 2016 21:28:18 +0200

The IP-over-1394 driver firewire-net lacked input validation when handling incoming fragmented datagrams. A maliciously formed fragment with a respectively large datagram_offset would cause a memcpy past the datagram buffer.

So, drop any packets carrying a fragment with offset + length larger than datagram_size.

In addition, ensure that
- GASP header, unfragmented encapsulation header, or fragment encapsulation header actually exists before we access it,
- the encapsulated datagram or fragment is of nonzero size.

---
 drivers/firewire/net.c |   51 ++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 16 deletions(-)

@@ -578,6 +578,9 @@ static int fwnet_incoming_packet(struct
 	int retval;
 	u16 ether_type;
 
+	if (len <= RFC2374_UNFRAG_HDR_SIZE)
+		return 0;
+
 	hdr.w0 = be32_to_cpu(buf[0]);
 	lf = fwnet_get_hdr_lf(&hdr);
 	if (lf == RFC2374_HDR_UNFRAG) {

@@ -602,7 +605,12 @@ static int fwnet_incoming_packet(struct
 		return fwnet_finish_incoming_packet(net, skb, source_node_id,
 						    is_broadcast, ether_type);
 	}
+
 	/* A datagram fragment has been received, now the fun begins. */
+
+	if (len <= RFC2374_FRAG_HDR_SIZE)
+		return 0;
+
 	hdr.w1 = ntohl(buf[1]);
 	buf += 2;
 	len -= RFC2374_FRAG_HDR_SIZE;

@@ -616,6 +624,9 @@ static int fwnet_incoming_packet(struct
 	datagram_label = fwnet_get_hdr_dgl(&hdr);
 	dg_size = fwnet_get_hdr_dg_size(&hdr); /* ??? + 1 */
 
+	if (fg_off + len > dg_size)
+		return 0;
+
 	spin_lock_irqsave(&dev->lock, flags);
 
 	peer = fwnet_peer_find_by_node_id(dev, source_node_id, generation);

@@ -722,6 +733,22 @@ static void fwnet_receive_packet(struct
 	fw_send_response(card, r, rcode);
 }
 
+static int gasp_source_id(__be32 *p)
+{
+	return be32_to_cpu(p[0]) >> 16;
+}
+
+static u32 gasp_specifier_id(__be32 *p)
+{
+	return (be32_to_cpu(p[0]) & 0xffff) << 8 |
+	       (be32_to_cpu(p[1]) & 0xff000000) >> 24;
+}
+
+static u32 gasp_version(__be32 *p)
+{
+	return be32_to_cpu(p[1]) & 0xffffff;
+}
+
 static void fwnet_receive_broadcast(struct fw_iso_context *context,
 		u32 cycle, size_t header_length, void *header, void *data)
 {

@@ -731,9 +758,6 @@ static void fwnet_receive_broadcast(stru
 	__be32 *buf_ptr;
 	int retval;
 	u32 length;
-	u16 source_node_id;
-	u32 specifier_id;
-	u32 ver;
 	unsigned long offset;
 	unsigned long flags;
 

@@ -750,22 +774,17 @@ static void fwnet_receive_broadcast(stru
 
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-	specifier_id =    (be32_to_cpu(buf_ptr[0]) & 0xffff) << 8
-			| (be32_to_cpu(buf_ptr[1]) & 0xff000000) >> 24;
-	ver = be32_to_cpu(buf_ptr[1]) & 0xffffff;
-	source_node_id = be32_to_cpu(buf_ptr[0]) >> 16;
-
-	if (specifier_id == IANA_SPECIFIER_ID &&
-	    (ver == RFC2734_SW_VERSION
+	if (length > IEEE1394_GASP_HDR_SIZE &&
+	    gasp_specifier_id(buf_ptr) == IANA_SPECIFIER_ID &&
+	    (gasp_version(buf_ptr) == RFC2734_SW_VERSION
 #if IS_ENABLED(CONFIG_IPV6)
-	     || ver == RFC3146_SW_VERSION
+	     || gasp_version(buf_ptr) == RFC3146_SW_VERSION
 #endif
-	    )) {
-		buf_ptr += 2;
-		length -= IEEE1394_GASP_HDR_SIZE;
-		fwnet_incoming_packet(dev, buf_ptr, length, source_node_id,
+	    ))
+		fwnet_incoming_packet(dev, buf_ptr + 2,
+				      length - IEEE1394_GASP_HDR_SIZE,
+				      gasp_source_id(buf_ptr),
 				      context->card->generation, true);
-	}
 
 	packet.payload_length = dev->rcv_buffer_size;
 	packet.interrupt = 1;

Stefan Richter
-======----- =-== ---=-
http://arcgraph.de/sr/

Date: Sun, 30 Oct 2016 17:32:01 +0100

RFC 2734 defines the datagram_size field in fragment encapsulation headers thus:

datagram_size: The encoded size of the entire IP datagram. The value of datagram_size [...] SHALL be one less than the value of Total Length in the datagram's IP header (see STD 5, RFC 791).

Accordingly, the eth1394 driver of Linux 2.6.36 and older set and got this field with a -/+1 offset:

    ether1394_tx() /* transmit */
        ether1394_encapsulate_prep()
            hdr->ff.dg_size = dg_size - 1;

    ether1394_data_handler() /* receive */
        if (hdr->common.lf == ETH1394_HDR_LF_FF)
            dg_size = hdr->ff.dg_size + 1;
        else
            dg_size = hdr->sf.dg_size + 1;

Likewise, I observe OS X 10.4 and Windows XP Pro SP3 to transmit 1500 byte sized datagrams in fragments with datagram_size=1499 if link fragmentation is required.

Only firewire-net sets and gets datagram_size without this offset. The result is lacking interoperability of firewire-net with OS X, Windows XP, and presumably Linux' eth1394. (I did not test with the latter.) For example, FTP data transfers to a Linux firewire-net box with max_rec smaller than the 1500 bytes MTU
- from OS X fail entirely,
- from Win XP start out with a bunch of fragmented datagrams which time out, then continue with unfragmented datagrams because Win XP temporarily reduces the MTU to 576 bytes.

So let's fix firewire-net's datagram_size accessors.

Note that firewire-net thereby loses interoperability with unpatched firewire-net, but only if link fragmentation is employed. (This happens with large broadcast datagrams, and with large datagrams on several FireWire CardBus cards with smaller max_rec than equivalent PCI cards, and it can be worked around by setting a small enough MTU.)

---
 drivers/firewire/net.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

@@ -73,13 +73,13 @@ struct rfc2734_header {
 
 #define fwnet_get_hdr_lf(h)		(((h)->w0 & 0xc0000000) >> 30)
 #define fwnet_get_hdr_ether_type(h)	(((h)->w0 & 0x0000ffff))
-#define fwnet_get_hdr_dg_size(h)	(((h)->w0 & 0x0fff0000) >> 16)
+#define fwnet_get_hdr_dg_size(h)	((((h)->w0 & 0x0fff0000) >> 16) + 1)
 #define fwnet_get_hdr_fg_off(h)		(((h)->w0 & 0x00000fff))
 #define fwnet_get_hdr_dgl(h)		(((h)->w1 & 0xffff0000) >> 16)
 
-#define fwnet_set_hdr_lf(lf)		((lf)  << 30)
+#define fwnet_set_hdr_lf(lf)		((lf) << 30)
 #define fwnet_set_hdr_ether_type(et)	(et)
-#define fwnet_set_hdr_dg_size(dgs)	((dgs) << 16)
+#define fwnet_set_hdr_dg_size(dgs)	(((dgs) - 1) << 16)
 #define fwnet_set_hdr_fg_off(fgo)	(fgo)
 
 #define fwnet_set_hdr_dgl(dgl)		((dgl) << 16)

@@ -622,7 +622,7 @@ static int fwnet_incoming_packet(struct
 		fg_off = fwnet_get_hdr_fg_off(&hdr);
 	}
 	datagram_label = fwnet_get_hdr_dgl(&hdr);
-	dg_size = fwnet_get_hdr_dg_size(&hdr); /* ??? + 1 */
+	dg_size = fwnet_get_hdr_dg_size(&hdr);
 
 	if (fg_off + len > dg_size)
 		return 0;

Stefan Richter
-======----- =-== ---=-
http://arcgraph.de/sr/

The latest max_mtu patch missed that datagram_size is actually one less than the datagram's Total Length.

---
To be applied after net-next was merged.

 drivers/firewire/net.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 5d3640264f2d..655c259e37fd 100644
@@ -1482,9 +1482,14 @@ static int fwnet_probe(struct fw_unit *unit,
 		goto out;
 	dev->local_fifo = dev->handler.offset;
 
+	/*
+	 * default MTU: RFC 2734 cl. 4, RFC 3146 cl. 4
+	 * maximum MTU: RFC 2734 cl. 4.2, fragment encapsulation header's
+	 *              maximum possible datagram_size + 1 = 0xfff + 1
+	 */
 	net->mtu = 1500U;
 	net->min_mtu = ETH_MIN_MTU;
-	net->max_mtu = 0xfff;
+	net->max_mtu = 4096U;
 
 	/* Set our hardware address while we're at it */
 	ha = (union fwnet_hwaddr *)net->dev_addr;

Stefan Richter
-======----- =-== ---=-
http://arcgraph.de/sr/