Skip to contentSkip to conversation

ip_queue and fragments

PATCH In review Pablo Neira 2 participants 4 messages Aug 5, 2004 – Aug 7, 2004

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Pablo Neira

author
#0 lore raw

PATCH 1/1 ip_queue and fragments

Hi,

I've tracing ip_queue source code and I think that it's not fragment aware. Am I missing anything?

regards,
Pablo

net/ipv4/netfilter/ip_queue.c +4 -3
diff -u -r1.1.1.1 ip_queue.c
@@ -255,9 +255,10 @@
 				entry->skb->dev->hard_header_parse(entry->skb,
 				                                   pmsg->hw_addr);
 	}
-	
-	if (data_len)
-		memcpy(pmsg->payload, entry->skb->data, data_len);
+
+	if (data_len) 
+		if (skb_copy_bits(entry->skb, 0, pmsg->payload, data_len) != 0)
+			goto nlmsg_failure;
 		
 	nlh->nlmsg_len = skb->tail - old_tail;
 	return skb;
#1 lore raw
Quoted 3 lines · Pablo Neira

Hi,

I've tracing ip_queue source code and I think that it's not fragment aware. Am I missing anything?

This check prevents copying data outside of skb->data:

       case IPQ_COPY_PACKET:
               if (copy_range == 0 || copy_range > entry->skb->len)
                       data_len = entry->skb->len;
               else
                       data_len = copy_range;

skb_copy_bits always stays inside limits if len is positive, so you can do something like this:

if (copy_range == 0)
data_len = ~0UL;
else
data_len = copy_range;

but you have to remove the jump to nlmsg_failure when skb_copy_bits fails.

Regards
Patrick

Quoted 21 lines

------------------------------------------------------------------------

diff -u -r1.1.1.1 ip_queue.c

net/ipv4/netfilter/ip_queue.c +4 -3
@@ -255,9 +255,10 @@
 				entry->skb->dev->hard_header_parse(entry->skb,
 				                                   pmsg->hw_addr);
 	}
-	
-	if (data_len)
-		memcpy(pmsg->payload, entry->skb->data, data_len);
+
+	if (data_len) 
+		if (skb_copy_bits(entry->skb, 0, pmsg->payload, data_len) != 0)
+			goto nlmsg_failure;
 
 	nlh->nlmsg_len = skb->tail - old_tail;
 	return skb;
author
#2 lore raw

Hi Patrick,

Quoted 9 lines · Patrick McHardy

skb_copy_bits always stays inside limits if len is positive, so you can do something like this:

if (copy_range == 0)
data_len = ~0UL;
else
data_len = copy_range;

but you have to remove the jump to nlmsg_failure when skb_copy_bits fails.

thanks for your suggestions, I've applied them to my patch.

regards,
Pablo

net/ipv4/netfilter/ip_queue.c +3 -3
diff -u -r1.1.1.1 ip_queue.c
@@ -205,8 +205,8 @@
 		break;
 	
 	case IPQ_COPY_PACKET:
-		if (copy_range == 0 || copy_range > entry->skb->len)
-			data_len = entry->skb->len;
+		if (copy_range == 0)
+			data_len = ~0UL;
 		else
 			data_len = copy_range;
 		

@@ -257,7 +257,7 @@
 	}
 	
 	if (data_len)
-		memcpy(pmsg->payload, entry->skb->data, data_len);
+		skb_copy_bits(entry->skb, 0, pmsg->payload, data_len);
 		
 	nlh->nlmsg_len = skb->tail - old_tail;
 	return skb;
#3 lore raw
Quoted 1 line · Pablo Neira

thanks for your suggestions, I've applied them to my patch.

Actually your first patch was fine and my suggestion was stupid ;) I'm going to put it in pom-ng later tonight.

Regards
Patrick