Skip to contentSkip to conversation

isdn: avm: potential signedness issue in loading firmware

PATCH In review Dan Carpenter 2 participants 4 messages May 9, 2014 1 message not in the archive

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Dan Carpenter

author
#1 lore raw

PATCH 1/1 isdn: avm: potential signedness issue in loading firmware

In reply to a message not in the archive

The concern here is that a negative value for "len" could lead underflow the "while (left > FWBUF_SIZE) {" test and lead to memory corruption. I do not believe this is possible because we test for negatives in old_capi_manufacturer(), "if (ldef.t4file.len <= 0) {".

But it sort of makes the code nicer to only deal with positive lengths so I have made it unsigned. The length is still not capped and so if we get a too large value, we keep on writing it out to the firmware byte by byte until the writes start failing.

Loading the firmware requires CAP_SYS_ADMIN.

Reported-by Henry Hoggard <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
drivers/isdn/hardware/avm/b1.c +2 -1
index 4d9b195..7f2ed49 100644
@@ -153,7 +153,8 @@ int b1_load_t4file(avmcard *card, capiloaddatapart *t4file)
 {
 	unsigned char buf[FWBUF_SIZE];
 	unsigned char *dp;
-	int i, left;
+	unsigned int left;
+	int i;
 	unsigned int base = card->port;
 
 	dp = t4file->data;
maintainer
#2 lore raw

From: Dan Carpenter <[email protected]>
Date: Fri, 9 May 2014 14:54:37 +0300

Quoted 20 lines

The concern here is that a negative value for "len" could lead underflow the "while (left > FWBUF_SIZE) {" test and lead to memory corruption. I do not believe this is possible because we test for negatives in old_capi_manufacturer(), "if (ldef.t4file.len <= 0) {".

But it sort of makes the code nicer to only deal with positive lengths so I have made it unsigned. The length is still not capped and so if we get a too large value, we keep on writing it out to the firmware byte by byte until the writes start failing.

Loading the firmware requires CAP_SYS_ADMIN.

Reported-by Henry Hoggard <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
drivers/isdn/hardware/avm/b1.c +0 -0
index 4d9b195..7f2ed49 100644

drivers/isdn/hardware/avm/b1.c int b1_load_t4file(avmcard *card, capiloaddatapart *t4file) · line 158

Quoted 7 lines
drivers/isdn/hardware/avm/b1.c +2 -1
@@ -153,7 +153,8 @@ int b1_load_t4file(avmcard *card, capiloaddatapart *t4file)
 {
 	unsigned char buf[FWBUF_SIZE];
 	unsigned char *dp;
-	int i, left;
+	unsigned int left;
+	int i;
 	unsigned int base = card->port;

Are you sure this is an improvement?

As you say, if we get a negative length, we'll run that "while (left > FWBUF_SIZE)" loop a LOT.

Whereas if you keep the signedness, it won't run even one time if we somehow were given a negative t4file->len.

To me, the only real option is to make this function itself guard against negative lengths, and return -EINVAL if it sees one.

Then you can perhaps also kill such checks from the callers.

author
#3 lore raw
Quoted 36 lines · David Miller

From: Dan Carpenter <[email protected]>
Date: Fri, 9 May 2014 14:54:37 +0300

Quoted 20 lines

The concern here is that a negative value for "len" could lead underflow the "while (left > FWBUF_SIZE) {" test and lead to memory corruption. I do not believe this is possible because we test for negatives in old_capi_manufacturer(), "if (ldef.t4file.len <= 0) {".

But it sort of makes the code nicer to only deal with positive lengths so I have made it unsigned. The length is still not capped and so if we get a too large value, we keep on writing it out to the firmware byte by byte until the writes start failing.

Loading the firmware requires CAP_SYS_ADMIN.

Reported-by Henry Hoggard <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
drivers/isdn/hardware/avm/b1.c +0 -0
index 4d9b195..7f2ed49 100644

drivers/isdn/hardware/avm/b1.c int b1_load_t4file(avmcard *card, capiloaddatapart *t4file) · line 158

Quoted 7 lines
drivers/isdn/hardware/avm/b1.c +2 -1
@@ -153,7 +153,8 @@ int b1_load_t4file(avmcard *card, capiloaddatapart *t4file)
 {
 	unsigned char buf[FWBUF_SIZE];
 	unsigned char *dp;
-	int i, left;
+	unsigned int left;
+	int i;
 	unsigned int base = card->port;

Are you sure this is an improvement?

As you say, if we get a negative length, we'll run that "while (left > FWBUF_SIZE)" loop a LOT.

Quoted 7 lines

Whereas if you keep the signedness, it won't run even one time if we somehow were given a negative t4file->len.

To me, the only real option is to make this function itself guard against negative lengths, and return -EINVAL if it sees one.

Then you can perhaps also kill such checks from the callers.

I would be fine if we did nothing here and left the code as is.

If we're going to kill duplicative checks then we should keep the current code. The check is in the one central place directly after we get the length from the user.

Adding a check for negative would make the code scanner happy but it's a bit meaningless because we don't have a way to know what the upper limit should be. We just keep writing the firmware until we're done or b1_save_put_byte() returns an error code.

Let's just leave this as is.

regards,
dan carpenter