Skip to contentSkip to conversation

ISDN: pcbit: off by one bugs in pcbit_set_msn()

PATCH Applied Dan Carpenter 2 participants 2 messages Jul 31, 2014 – Aug 1, 2014

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

PATCH 1/1 ISDN: pcbit: off by one bugs in pcbit_set_msn()

1) We don't allocate enough space for the NUL terminator so we end up corrupting one character beyond the end of the buffer.

2) The "len - 1" should just be "len". The code is trying to copy a word from a buffer up to a comma or the last word in the buffer. Say you have the buffer, "foo,bar,baz", then this code truncates the last letter off each word so you get "fo", "ba", and "ba". You would hope this kind of bug would get noticed in testing...

I'm not very familiar with this code and I can't test it, but I think we should copy the final character.

drivers/isdn/pcbit/drv.c +2 -2
index f02cc50..4172e22 100644
@@ -1035,14 +1035,14 @@ static void pcbit_set_msn(struct pcbit_dev *dev, char *list)
 		}
 		ptr->next = NULL;
 
-		ptr->msn = kmalloc(len, GFP_ATOMIC);
+		ptr->msn = kmalloc(len + 1, GFP_ATOMIC);
 		if (!ptr->msn) {
 			printk(KERN_WARNING "kmalloc failed\n");
 			kfree(ptr);
 			return;
 		}
 
-		memcpy(ptr->msn, sp, len - 1);
+		memcpy(ptr->msn, sp, len);
 		ptr->msn[len] = 0;
 
 #ifdef DEBUG
maintainer Applied
#1 lore raw

From: Dan Carpenter <[email protected]>
Date: Thu, 31 Jul 2014 18:30:11 +0300

Quoted 9 lines

1) We don't allocate enough space for the NUL terminator so we end up corrupting one character beyond the end of the buffer.

2) The "len - 1" should just be "len". The code is trying to copy a word from a buffer up to a comma or the last word in the buffer. Say you have the buffer, "foo,bar,baz", then this code truncates the last letter off each word so you get "fo", "ba", and "ba". You would hope this kind of bug would get noticed in testing...

Quoted 4 lines

I'm not very familiar with this code and I can't test it, but I think we should copy the final character.

Applied to net-next, thanks Dan.