Skip to contentSkip to conversation

af_packet: check loop for greater than zero in tpacket_fill_skb

PATCH In review Daniel Borkmann 2 participants 2 messages Jun 10, 2012 – Jun 11, 2012

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 af_packet: check loop for greater than zero in tpacket_fill_skb

It could be more safe to check the 'to_write' for 'greater than zero' instead for 'not zero'. 'to_write' is of type int and subtraction operations are performed on it, so in the case of malformed values that are subtracted from 'to_write', it could become less than zero, which is then interpreted as 'not zero' in the while condition, thus the loop won't return as expected.

---
 net/packet/af_packet.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/packet/af_packet.c +1 -1
index 0f66174..3e53680 100644
@@ -2027,7 +2027,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 	skb->truesize += to_write;
 	atomic_add(to_write, &po->sk.sk_wmem_alloc);
 
-	while (likely(to_write)) {
+	while (likely(to_write > 0)) {
 		nr_frags = skb_shinfo(skb)->nr_frags;
 
 		if (unlikely(nr_frags >= MAX_SKB_FRAGS)) {
maintainer
#1 lore raw

From: Daniel Borkmann <[email protected]>
Date: Sun, 10 Jun 2012 21:25:08 +0200

Quoted 8 lines

It could be more safe to check the 'to_write' for 'greater than zero' instead for 'not zero'. 'to_write' is of type int and subtraction operations are performed on it, so in the case of malformed values that are subtracted from 'to_write', it could become less than zero, which is then interpreted as 'not zero' in the while condition, thus the loop won't return as expected.

I hate this kind of change.

The implication is that there are bugs in the loop that can cause the value to go negative.

Unless you can show that this actually happens, leave this clean test alone.