* [patch] isdn: strcpy() => strlcpy()
@ 2010-10-05 16:34 Dan Carpenter
2010-10-05 16:43 ` Al Viro
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2010-10-05 16:34 UTC (permalink / raw)
To: Karsten Keil; +Cc: netdev, kernel-janitors
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.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/drivers/isdn/sc/interrupt.c b/drivers/isdn/sc/interrupt.c
index 485be8b..288073e 100644
--- a/drivers/isdn/sc/interrupt.c
+++ b/drivers/isdn/sc/interrupt.c
@@ -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;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [patch] isdn: strcpy() => strlcpy()
2010-10-05 16:34 [patch] isdn: strcpy() => strlcpy() Dan Carpenter
@ 2010-10-05 16:43 ` Al Viro
2010-10-06 5:17 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Al Viro @ 2010-10-05 16:43 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Karsten Keil, netdev, kernel-janitors
On Tue, Oct 05, 2010 at 06:34:48PM +0200, Dan Carpenter wrote:
> 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.
ITYM "I papered it over, so potentially broken behaviour is harder to
find now"...
> - 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?
> + strlcpy(setup.eazmsn,
> + sc_adapter[card]->channel[rcvmsg.phy_link_no - 1].dn,
> + sizeof(setup.eazmsn));
Ditto.
> - 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [patch] isdn: strcpy() => strlcpy()
2010-10-05 16:43 ` Al Viro
@ 2010-10-06 5:17 ` Dan Carpenter
2010-10-08 17:23 ` David Miller
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2010-10-06 5:17 UTC (permalink / raw)
To: Al Viro; +Cc: Karsten Keil, netdev, kernel-janitors
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.
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
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.
diff --git a/drivers/isdn/sc/interrupt.c b/drivers/isdn/sc/interrupt.c
index 485be8b..f0225bc 100644
--- a/drivers/isdn/sc/interrupt.c
+++ b/drivers/isdn/sc/interrupt.c
@@ -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;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [patch] isdn: strcpy() => strlcpy()
2010-10-06 5:17 ` Dan Carpenter
@ 2010-10-08 17:23 ` David Miller
0 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2010-10-08 17:23 UTC (permalink / raw)
To: error27; +Cc: viro, isdn, netdev, kernel-janitors
From: Dan Carpenter <error27@gmail.com>
Date: Wed, 6 Oct 2010 07:17:35 +0200
> 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.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Applied, thanks Dan.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-10-08 17:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-05 16:34 [patch] isdn: strcpy() => strlcpy() Dan Carpenter
2010-10-05 16:43 ` Al Viro
2010-10-06 5:17 ` Dan Carpenter
2010-10-08 17:23 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).