isdn: strcpy() => strlcpy()
Subsystems (3)
PATCH 1/1 isdn: strcpy() => strlcpy()
setup.phone and setup.eazmsn are 32 character buffers. rcvmsg.msg_data.byte_array is a 48 character buffer. sc_adapter[card]->channel[rcvmsg.phy_link_no - 1].dn is 50 chars.
I changed the strcpy() so strlcpy() because that's safest.
drivers/isdn/sc/interrupt.c +8 -4
@@ -114,9 +114,11 @@ irqreturn_t interrupt_handler(int dummy, void *card_inst) {pr_debug ("%s: Got Incoming Call\n" , sc_adapter[card]->devicename); -strcpy (setup.phone,&(rcvmsg.msg_data.byte_array[4 ])); -strcpy (setup.eazmsn, - sc_adapter[card]->channel[rcvmsg.phy_link_no-1 ].dn); +strlcpy (setup.phone, &(rcvmsg.msg_data.byte_array[4 ]), +sizeof (setup.phone)); +strlcpy (setup.eazmsn, + sc_adapter[card]->channel[rcvmsg.phy_link_no -1 ].dn, +sizeof (setup.eazmsn)); setup.si1 =7 ; setup.si2 =0 ; setup.plan =0 ;
@@ -176,7 +178,9 @@ irqreturn_t interrupt_handler(int dummy, void *card_inst)* Handle a GetMyNumber Rsp */ if (IS_CE_MESSAGE (rcvmsg,Call,0 ,GetMyNumber)){ -strcpy (sc_adapter[card]->channel[rcvmsg.phy_link_no-1 ].dn,rcvmsg.msg_data.byte_array); +strlcpy (sc_adapter[card]->channel[rcvmsg.phy_link_no -1 ].dn, + rcvmsg.msg_data.byte_array, +sizeof (rcvmsg.msg_data.byte_array));continue ; }
Quoted 5 lines · Dan Carpenter
setup.phone and setup.eazmsn are 32 character buffers. rcvmsg.msg_data.byte_array is a 48 character buffer. sc_adapter[card]->channel[rcvmsg.phy_link_no - 1].dn is 50 chars.
I changed the
strcpy()sostrlcpy()because that's safest.
ITYM "I papered it over, so potentially broken behaviour is harder to find now"...
drivers/isdn/sc/interrupt.c irqreturn_t interrupt_handler(int dummy, void *card_inst) · line 118
Quoted 5 lines
drivers/isdn/sc/interrupt.c +2
-3-strcpy (setup.phone,&(rcvmsg.msg_data.byte_array[4 ])); -strcpy (setup.eazmsn, - sc_adapter[card]->channel[rcvmsg.phy_link_no-1 ].dn); +strlcpy (setup.phone, &(rcvmsg.msg_data.byte_array[4 ]), +sizeof (setup.phone));
OK, so what should be done if the damn array contents _is_ longer than that?
drivers/isdn/sc/interrupt.c irqreturn_t interrupt_handler(int dummy, void *card_inst) · line 121
Quoted 3 lines
drivers/isdn/sc/interrupt.c +3
-0+strlcpy (setup.eazmsn, + sc_adapter[card]->channel[rcvmsg.phy_link_no -1 ].dn, +sizeof (setup.eazmsn));
Ditto.
drivers/isdn/sc/interrupt.c irqreturn_t interrupt_handler(int dummy, void *card_inst) · line 183
Quoted 4 lines
drivers/isdn/sc/interrupt.c +3
-1-strcpy (sc_adapter[card]->channel[rcvmsg.phy_link_no-1 ].dn,rcvmsg.msg_data.byte_array); +strlcpy (sc_adapter[card]->channel[rcvmsg.phy_link_no -1 ].dn, + rcvmsg.msg_data.byte_array, +sizeof (rcvmsg.msg_data.byte_array));
Huh? Is it or is it not NUL-terminated? If it is, then change is pure cargo-culting; if it is not, you are asking for nasal daemons to fly.
PATCH 1/1 isdn: strcpy() => strlcpy()
setup.phone and setup.eazmsn are 32 character buffers. rcvmsg.msg_data.byte_array is a 48 character buffer. sc_adapter[card]->channel[rcvmsg.phy_link_no - 1].dn is 50 chars.
The rcvmsg struct comes from the memcpy_fromio() in receivemessage(). I guess that means it's data off the wire. I'm not very familiar with this code but I don't see any reason to assume these strings are NULL terminated.
Also it's weird that "dn" in a 50 character buffer but we only seem to use 32 characters. In drivers/isdn/sc/scioc.h, "dn" is only a 49 character buffer. So potentially there is still an issue there.
The important thing for now is to prevent the memory corruption.
---
I don't have this hardware, but this patch should not introduce any bugs that weren't there in the original.
v2: If the strlcpy() strings aren't NULL terminated then bail out
earlier. Add a better commit message. The first commit message sucked. Sorry for that.
drivers/isdn/sc/interrupt.c +14 -4
@@ -112,11 +112,19 @@ irqreturn_t interrupt_handler(int dummy, void *card_inst) }else if (callid>=0x0000 && callid<=0x7FFF ) { +int len; +pr_debug ("%s: Got Incoming Call\n" , sc_adapter[card]->devicename); -strcpy (setup.phone,&(rcvmsg.msg_data.byte_array[4 ])); -strcpy (setup.eazmsn, - sc_adapter[card]->channel[rcvmsg.phy_link_no-1 ].dn); + len =strlcpy (setup.phone, &(rcvmsg.msg_data.byte_array[4 ]), +sizeof (setup.phone)); +if (len >=sizeof (setup.phone)) +continue ; + len =strlcpy (setup.eazmsn, + sc_adapter[card]->channel[rcvmsg.phy_link_no -1 ].dn, +sizeof (setup.eazmsn)); +if (len >=sizeof (setup.eazmsn)) +continue ; setup.si1 =7 ; setup.si2 =0 ; setup.plan =0 ;
@@ -176,7 +184,9 @@ irqreturn_t interrupt_handler(int dummy, void *card_inst)* Handle a GetMyNumber Rsp */ if (IS_CE_MESSAGE (rcvmsg,Call,0 ,GetMyNumber)){ -strcpy (sc_adapter[card]->channel[rcvmsg.phy_link_no-1 ].dn,rcvmsg.msg_data.byte_array); +strlcpy (sc_adapter[card]->channel[rcvmsg.phy_link_no -1 ].dn, + rcvmsg.msg_data.byte_array, +sizeof (rcvmsg.msg_data.byte_array));continue ; }
From: Dan Carpenter <[email protected]>
Date: Wed, 6 Oct 2010 07:17:35 +0200
Quoted 9 lines
setup.phone and setup.eazmsn are 32 character buffers. rcvmsg.msg_data.byte_array is a 48 character buffer. sc_adapter[card]->channel[rcvmsg.phy_link_no - 1].dn is 50 chars.
The rcvmsg struct comes from the
memcpy_fromio()inreceivemessage(). I guess that means it's data off the wire. I'm not very familiar with this code but I don't see any reason to assume these strings are NULL terminated.
Quoted 7 lines
Also it's weird that "dn" in a 50 character buffer but we only seem to use 32 characters. In
drivers/isdn/sc/scioc.h, "dn" is only a 49 character buffer. So potentially there is still an issue there.The important thing for now is to prevent the memory corruption.
Signed-off-by Dan Carpenter <[email protected]>
Applied, thanks Dan.