Bug in sms

Bug in sms

[Gu, Yang]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

Hi,
	Today I tried oFono with my cell phone, but it crashed when starting up. The problem happens in function at_cmgl_notify() of file drivers/atmodem/sms.c. In my case, strlen(hexpdu) == 338, but the buffer "pdu" has maximum size of 164. So after decode_hex_own_buf(), some memory was written unexpectedly. Call for a fix, please.


Regards,
-Yang

Re: Bug in sms

[Dennis.Yxun]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

HI Yang:
    Mind I ask you a question? maybe off-topic.
Which hardware(the GSM modem?) are you testing?
Is that possible for a developer to get an open hardware,
So they can put their hands in
   Thanks.

Dennis

On Wed, Sep 16, 2009 at 8:22 AM, Gu, Yang <yang.gu@intel.com> wrote:

> Hi,
>        Today I tried oFono with my cell phone, but it crashed when starting
> up. The problem happens in function at_cmgl_notify() of file
> drivers/atmodem/sms.c. In my case, strlen(hexpdu) == 338, but the buffer
> "pdu" has maximum size of 164. So after decode_hex_own_buf(), some memory
> was written unexpectedly. Call for a fix, please.
>
>
> Regards,
> -Yang
>
>
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono
>

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 1225 bytes --]

RE: Bug in sms

[Gu, Yang]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

Hi Dennis,
I am just using my cell phone (Sony Ericsson T618, GSM). Not sure what's your meaning by "open hardware". oFono is gradually adding support for some modems, such as HTC G1, Ericsson MBM, TI Calypso, etc.
To my understanding, different modems have different implementation, in which some follows the spec, while others not. I think oFono's target is to support modems as many as possible, if they follow the spec well. For this problem, I don't know if my modem is a good one to follow spec. However, oFono needs more error handling to avoid segment fault.


Regards,
-Yang

From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of Dennis.Yxun
Sent: Wednesday, September 16, 2009 5:22 PM
To: ofono(a)ofono.org
Subject: Re: Bug in sms

HI Yang:
    Mind I ask you a question? maybe off-topic.
Which hardware(the GSM modem?) are you testing?
Is that possible for a developer to get an open hardware,
So they can put their hands in
   Thanks.

Dennis
On Wed, Sep 16, 2009 at 8:22 AM, Gu, Yang <yang.gu<http://yang.gu>@intel.com<http://intel.com>> wrote:
Hi,
       Today I tried oFono with my cell phone, but it crashed when starting up. The problem happens in function at_cmgl_notify() of file drivers/atmodem/sms.c. In my case, strlen(hexpdu) == 338, but the buffer "pdu" has maximum size of 164. So after decode_hex_own_buf(), some memory was written unexpectedly. Call for a fix, please.


Regards,
-Yang


_______________________________________________
ofono mailing list
ofono(a)ofono.org<mailto:ofono@ofono.org>
http://lists.ofono.org/listinfo/ofono


[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 8294 bytes --]

Re: Bug in sms

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 83 bytes --]

> Oops, forgot to attach.

Patch has been applied.  Thanks.

Regards,
-Denis

Re: Bug in sms

[Andrzej Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 558 bytes --]

Hi,

2009/9/16 Gu, Yang <yang.gu@intel.com>:
>        Today I tried oFono with my cell phone, but it crashed when starting up.
> The problem happens in function at_cmgl_notify() of file drivers/atmodem/sms.c.
> In my case, strlen(hexpdu) == 338, but the buffer "pdu" has maximum size of
> 164. So after decode_hex_own_buf(), some memory was written unexpectedly.

The attached patch adds length check everywehere decode_hex_own_buf()
is used with a static buffer and also enlarges the buffers to account
for SMSC included in a PDU.

Regards

Re: Bug in sms

[Andrzej Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 25 bytes --]

Oops, forgot to attach.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Check-received-PDUs-fit-in-the-buffer-fix-buffer-si.patch --]
[-- Type: text/x-patch, Size: 2269 bytes --]

From b5ed486ad94fa549d8f30ad054e7edcb52560301 Mon Sep 17 00:00:00 2001
From: Andrzej Zaborowski <andrew.zaborowski@intel.com>
Date: Wed, 16 Sep 2009 16:03:50 +0200
Subject: [PATCH] Check received PDUs fit in the buffer, fix buffer size.

---
 drivers/atmodem/sms.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index 3b7e9e4..d425818 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -314,7 +314,7 @@ static void at_cmt_notify(GAtResult *result, gpointer user_data)
 	const char *hexpdu;
 	long pdu_len;
 	int tpdu_len;
-	unsigned char pdu[164];
+	unsigned char pdu[176];
 	char buf[256];
 
 	dump_response("at_cmt_notify", TRUE, result);
@@ -324,6 +324,11 @@ static void at_cmt_notify(GAtResult *result, gpointer user_data)
 		return;
 	}
 
+	if (strlen(hexpdu) > sizeof(pdu) * 2) {
+		ofono_error("Bad PDU length in CMT notification");
+		return;
+	}
+
 	ofono_debug("Got new SMS Deliver PDU via CMT: %s, %d", hexpdu, tpdu_len);
 
 	decode_hex_own_buf(hexpdu, -1, &pdu_len, 0, pdu);
@@ -344,7 +349,7 @@ static void at_cmgr_notify(GAtResult *result, gpointer user_data)
 	struct ofono_sms *sms = user_data;
 	GAtResultIter iter;
 	const char *hexpdu;
-	unsigned char pdu[164];
+	unsigned char pdu[176];
 	long pdu_len;
 	int tpdu_len;
 
@@ -366,6 +371,9 @@ static void at_cmgr_notify(GAtResult *result, gpointer user_data)
 
 	hexpdu = g_at_result_pdu(result);
 
+	if (strlen(hexpdu) > sizeof(pdu) * 2)
+		goto err;
+
 	ofono_debug("Got PDU: %s, with len: %d", hexpdu, tpdu_len);
 
 	decode_hex_own_buf(hexpdu, -1, &pdu_len, 0, pdu);
@@ -485,7 +493,7 @@ static void at_cmgl_notify(GAtResult *result, gpointer user_data)
 	struct sms_data *data = ofono_sms_get_data(sms);
 	GAtResultIter iter;
 	const char *hexpdu;
-	unsigned char pdu[164];
+	unsigned char pdu[176];
 	long pdu_len;
 	int tpdu_len;
 	int index;
@@ -518,6 +526,9 @@ static void at_cmgl_notify(GAtResult *result, gpointer user_data)
 		ofono_debug("Found an old SMS PDU: %s, with len: %d",
 				hexpdu, tpdu_len);
 
+		if (strlen(hexpdu) > sizeof(pdu) * 2)
+			continue;
+
 		decode_hex_own_buf(hexpdu, -1, &pdu_len, 0, pdu);
 		ofono_sms_deliver_notify(sms, pdu, pdu_len, tpdu_len);
 
-- 
1.6.1