Re: [PATCH 3.2 144/152] firewire: net: guard against rx buffer overflows

[Stefan Richter]

On Nov 14 Ben Hutchings wrote:
> 3.2.84-rc1 review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Stefan Richter 
> 
> commit 667121ace9dbafb368618dbabcf07901c962ddac upstream.
[...]
> [bwh: Backported to 3.2: fwnet_receive_broadcast() never matches IPv6 packets]
> Signed-off-by: Ben Hutchings 

Backport looks correct to me; thanks.
-- 
Stefan Richter
-==- =-== -===-
http://arcgraph.de/sr/

Re: [PATCH 3.2 144/152] firewire: net: guard against rx buffer overflows

[Stefan Richter]

On Nov 14 Ben Hutchings wrote:
> 3.2.84-rc1 review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Stefan Richter 
> 
> commit 667121ace9dbafb368618dbabcf07901c962ddac upstream.
[...]
> [bwh: Backported to 3.2: fwnet_receive_broadcast() never matches IPv6 packets]
> Signed-off-by: Ben Hutchings 

Backport looks correct to me; thanks.
-- 
Stefan Richter
-==- =-== -===-
http://arcgraph.de/sr/

[PATCH 3.2 144/152] firewire: net: guard against rx buffer overflows

[Ben Hutchings]

3.2.84-rc1 review patch.  If anyone has any objections, please let me know.

--

From: Stefan Richter 

commit 667121ace9dbafb368618dbabcf07901c962ddac upstream.

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.

Reported-by: Eyal Itkin 
Reviewed-by: Eyal Itkin 
Fixes: CVE 2016-8633
Signed-off-by: Stefan Richter 
[bwh: Backported to 3.2: fwnet_receive_broadcast() never matches IPv6 packets]
Signed-off-by: Ben Hutchings 
---
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -677,6 +677,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();
if (lf == RFC2374_HDR_UNFRAG) {
@@ -702,7 +705,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;
@@ -716,6 +724,9 @@ static int fwnet_incoming_packet(struct
datagram_label = fwnet_get_hdr_dgl();
dg_size = fwnet_get_hdr_dg_size(); /* ??? + 1 */
 
+   if (fg_off + len > dg_size)
+   return 0;
+
spin_lock_irqsave(>lock, flags);
 
peer = fwnet_peer_find_by_node_id(dev, source_node_id, generation);
@@ -822,6 +833,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]) & 0x) << 8 |
+  (be32_to_cpu(p[1]) & 0xff00) >> 24;
+}
+
+static u32 gasp_version(__be32 *p)
+{
+   return be32_to_cpu(p[1]) & 0xff;
+}
+
 static void fwnet_receive_broadcast(struct fw_iso_context *context,
u32 cycle, size_t header_length, void *header, void *data)
 {
@@ -832,9 +859,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;
 
@@ -852,17 +876,13 @@ static void fwnet_receive_broadcast(stru
 
spin_unlock_irqrestore(>lock, flags);
 
-   specifier_id =(be32_to_cpu(buf_ptr[0]) & 0x) << 8
-   | (be32_to_cpu(buf_ptr[1]) & 0xff00) >> 24;
-   ver = be32_to_cpu(buf_ptr[1]) & 0xff;
-   source_node_id = be32_to_cpu(buf_ptr[0]) >> 16;
-
-   if (specifier_id == IANA_SPECIFIER_ID && ver == RFC2734_SW_VERSION) {
-   buf_ptr += 2;
-   length -= IEEE1394_GASP_HDR_SIZE;
-   fwnet_incoming_packet(dev, buf_ptr, length, source_node_id,
+   if (length > IEEE1394_GASP_HDR_SIZE &&
+   gasp_specifier_id(buf_ptr) == IANA_SPECIFIER_ID &&
+   gasp_version(buf_ptr) == RFC2734_SW_VERSION)
+   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;

[PATCH 3.2 144/152] firewire: net: guard against rx buffer overflows

[Ben Hutchings]

3.2.84-rc1 review patch.  If anyone has any objections, please let me know.

--

From: Stefan Richter 

commit 667121ace9dbafb368618dbabcf07901c962ddac upstream.

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.

Reported-by: Eyal Itkin 
Reviewed-by: Eyal Itkin 
Fixes: CVE 2016-8633
Signed-off-by: Stefan Richter 
[bwh: Backported to 3.2: fwnet_receive_broadcast() never matches IPv6 packets]
Signed-off-by: Ben Hutchings 
---
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -677,6 +677,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();
if (lf == RFC2374_HDR_UNFRAG) {
@@ -702,7 +705,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;
@@ -716,6 +724,9 @@ static int fwnet_incoming_packet(struct
datagram_label = fwnet_get_hdr_dgl();
dg_size = fwnet_get_hdr_dg_size(); /* ??? + 1 */
 
+   if (fg_off + len > dg_size)
+   return 0;
+
spin_lock_irqsave(>lock, flags);
 
peer = fwnet_peer_find_by_node_id(dev, source_node_id, generation);
@@ -822,6 +833,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]) & 0x) << 8 |
+  (be32_to_cpu(p[1]) & 0xff00) >> 24;
+}
+
+static u32 gasp_version(__be32 *p)
+{
+   return be32_to_cpu(p[1]) & 0xff;
+}
+
 static void fwnet_receive_broadcast(struct fw_iso_context *context,
u32 cycle, size_t header_length, void *header, void *data)
 {
@@ -832,9 +859,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;
 
@@ -852,17 +876,13 @@ static void fwnet_receive_broadcast(stru
 
spin_unlock_irqrestore(>lock, flags);
 
-   specifier_id =(be32_to_cpu(buf_ptr[0]) & 0x) << 8
-   | (be32_to_cpu(buf_ptr[1]) & 0xff00) >> 24;
-   ver = be32_to_cpu(buf_ptr[1]) & 0xff;
-   source_node_id = be32_to_cpu(buf_ptr[0]) >> 16;
-
-   if (specifier_id == IANA_SPECIFIER_ID && ver == RFC2734_SW_VERSION) {
-   buf_ptr += 2;
-   length -= IEEE1394_GASP_HDR_SIZE;
-   fwnet_incoming_packet(dev, buf_ptr, length, source_node_id,
+   if (length > IEEE1394_GASP_HDR_SIZE &&
+   gasp_specifier_id(buf_ptr) == IANA_SPECIFIER_ID &&
+   gasp_version(buf_ptr) == RFC2734_SW_VERSION)
+   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;