[PATCH] drivers/isdn: checkng length to be sure not memory overflow

[PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Chen Gang]

  the length of cmd.parm.cmsg.para is 50 (MAX_CAPI_PARA_LEN).
  the strlen(msg) may be more than 50 (Modem-Commandbuffer, less than 255).
    isdn_tty_send_msg is called by isdn_tty_parse_at
    the relative parameter is m->mdmcmd (atemu *m)
    the relative command may be "+M..."

  so need check the length to be sure not memory overflow.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 drivers/isdn/i4l/isdn_tty.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
index d8a7d83..c3f0f99 100644
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c
@@ -902,7 +902,7 @@ isdn_tty_send_msg(modem_info *info, atemu *m, char *msg)
 	int j;
 	int l;
 
-	l = strlen(msg);
+	l = min(strlen(msg), sizeof(cmd.parm.cmsg.para) - 2);
 	if (!l) {
 		isdn_tty_modem_result(RESULT_ERROR, info);
 		return;
-- 
1.7.7.6

Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Jiri Slaby]

On 02/27/2013 10:05 AM, Chen Gang wrote:
> 
>   the length of cmd.parm.cmsg.para is 50 (MAX_CAPI_PARA_LEN).
>   the strlen(msg) may be more than 50 (Modem-Commandbuffer, less than 255).
>     isdn_tty_send_msg is called by isdn_tty_parse_at
>     the relative parameter is m->mdmcmd (atemu *m)
>     the relative command may be "+M..."
> 
>   so need check the length to be sure not memory overflow.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  drivers/isdn/i4l/isdn_tty.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
> index d8a7d83..c3f0f99 100644
> --- a/drivers/isdn/i4l/isdn_tty.c
> +++ b/drivers/isdn/i4l/isdn_tty.c
> @@ -902,7 +902,7 @@ isdn_tty_send_msg(modem_info *info, atemu *m, char *msg)
>  	int j;
>  	int l;
>  
> -	l = strlen(msg);
> +	l = min(strlen(msg), sizeof(cmd.parm.cmsg.para) - 2);
>  	if (!l) {
>  		isdn_tty_modem_result(RESULT_ERROR, info);
>  		return;
> 

Yeah, looks sensible from the buffer overflow POV. I have no idea if
this is correct from the ISDN POV as we drop the end of the buffer. But
who cares, when nobody noticed in the past decade...

thanks,
-- 
js
suse labs

Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Chen Gang]

于 2013年02月27日 17:48, Jiri Slaby 写道:
> I have no idea if
> this is correct from the ISDN POV as we drop the end of the buffer

  pardon ?  what about "ISDN POV".
  excuse me, my English is not quite well, I am either not quite
familiar with ISDN.

  thanks.

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Jiri Slaby]

On 02/27/2013 11:25 AM, Chen Gang wrote:
> 于 2013年02月27日 17:48, Jiri Slaby 写道:
>> I have no idea if
>> this is correct from the ISDN POV as we drop the end of the buffer
> 
>   pardon ?  what about "ISDN POV".

"point of view" aka POV.

Hmm, "also known as" alias "aka" :).

-- 
js
suse labs

Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Chen Gang]

于 2013年02月27日 18:44, Jiri Slaby 写道:
> On 02/27/2013 11:25 AM, Chen Gang wrote:
>> > 于 2013年02月27日 17:48, Jiri Slaby 写道:
>>> >> I have no idea if
>>> >> this is correct from the ISDN POV as we drop the end of the buffer
>> > 
>> >   pardon ?  what about "ISDN POV".
> "point of view" aka POV.
> 
> Hmm, "also known as" alias "aka" :).


  sorry, I still not quite understand (I am really not familiar with ISDN)

  so I have to bother you with 2 questions, please help reply, thanks.

    A) is our current patch OK ?
       a. yes, ok, need do nothing for it, just is waiting for Acked-by or applying.
       b. no, need improving (e.g. additional consideration, comments, or others)
       c. no, it is useless patch.

    B) does "ISDN POV" point to another issue ?
       if yes:
         I will read source code or search document on net, and should not bother you again.
         if possible, I can try to send relative patches, next.
       else (no):
         could you please say more details again ?
         I should improve current patch (also need let you as Signed-of-By)

  thanks.

  :-)

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Jiri Slaby]

On 02/28/2013 02:45 AM, Chen Gang wrote:
> 于 2013年02月27日 18:44, Jiri Slaby 写道:
>> On 02/27/2013 11:25 AM, Chen Gang wrote:
>>>> 于 2013年02月27日 17:48, Jiri Slaby 写道:
>>>>>> I have no idea if
>>>>>> this is correct from the ISDN POV as we drop the end of the buffer
>>>>
>>>>   pardon ?  what about "ISDN POV".
>> "point of view" aka POV.
>>
>> Hmm, "also known as" alias "aka" :).
> 
> 
>   sorry, I still not quite understand (I am really not familiar with ISDN)
> 
>   so I have to bother you with 2 questions, please help reply, thanks.
> 
>     A) is our current patch OK ?
>        a. yes, ok, need do nothing for it, just is waiting for Acked-by or applying.
>        b. no, need improving (e.g. additional consideration, comments, or others)
>        c. no, it is useless patch.
> 
>     B) does "ISDN POV" point to another issue ?
>        if yes:
>          I will read source code or search document on net, and should not bother you again.
>          if possible, I can try to send relative patches, next.
>        else (no):
>          could you please say more details again ?

No, the sentence was "I have no idea if this is correct from the ISDN
point of view because we drop the end of the buffer." I don't think
there are piles of people to care about ISDN much nowadays. So we can
close that it is correct to drop the rest of the buffer. In a hope that
+M is not followed by text longer than 50-or-so chars.

There is nothing more to fix. (Well, there is, but not in this context.)

-- 
js
suse labs

Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Chen Gang]

于 2013年02月28日 18:00, Jiri Slaby 写道:
> I don't think there are piles of people to care about ISDN much nowadays. 

  I don't think either.
    (I found it through reading the source code, by search strncpy)

  if this is quite minor:
    I suggest to delete this module.
    the reason is:
      it can not provide contributes, any more.
      but may give a chance to the hacker which want to make an attack.

  :-)

> So we can
> close that it is correct to drop the rest of the buffer. In a hope that
> +M is not followed by text longer than 50-or-so chars.

  can we be sure that "+M..." is no more than 100+ chars ?
    (I guess the sizeof (isdn_ctrl.parm) is 80+, but less than 100)
  if we can not be sure:
    do we need check and limit the length ? (I prefer to give a check)
    if the module will really be delete,
      I still suggest to maintain previous versions (for security issue)


  thanks.


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Jiri Slaby]

On 02/28/2013 12:01 PM, Chen Gang wrote:
> 于 2013年02月28日 18:00, Jiri Slaby 写道:
>> I don't think there are piles of people to care about ISDN much nowadays. 
> 
>   I don't think either.
>     (I found it through reading the source code, by search strncpy)
> 
>   if this is quite minor:
>     I suggest to delete this module.

Nah, there *are* still people using ISDN.

>> So we can
>> close that it is correct to drop the rest of the buffer. In a hope that
>> +M is not followed by text longer than 50-or-so chars.
> 
>   can we be sure that "+M..." is no more than 100+ chars ?
>     (I guess the sizeof (isdn_ctrl.parm) is 80+, but less than 100)
>   if we can not be sure:

No, we cannot be sure that a user gives us less than that. Your patch
just throws the rest to fix that overflow, right? What I'm saying I
wouldn't fix more than that.

-- 
js
suse labs

Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Chen Gang]

于 2013年02月28日 21:43, Jiri Slaby 写道:
> 
> Nah, there *are* still people using ISDN.
> 

  ok, thanks.

  it seems, we need maintaining ISDN:
    need fix bugs.
    need not add new features.
    need keep current features no touch.


> No, we cannot be sure that a user gives us less than that. Your patch
> just throws the rest to fix that overflow, right? What I'm saying I
> wouldn't fix more than that.

  what you said is: this patch need improving, is it correct ?
  if it is correct.
    I still prefer to throw the rest contents (but need improving, too)
      for maintaining, we need fix bug, but not need add new features
      so what we should do:
        a) fix the bug (should not memory overflow)
        b) keep the original buffer length no touch
           it is not sizeof(cmd.parm.cmsg.para) - 2)
           it should be sizeof(cmd.param) - sizeof(cmd.param.cmsg) + sizeof(cmd.param.cmsg.para) - 2
        c) need complete the relative document to export the limitation.

  is it ok ?
    (if it is not ok, welcome to provide suggestion or completion)


  thanks.

  :-)

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Jiri Slaby]

On 03/05/2013 03:20 AM, Chen Gang wrote:
> 于 2013年02月28日 21:43, Jiri Slaby 写道:
>>
>> Nah, there *are* still people using ISDN.
>>
> 
>   ok, thanks.
> 
>   it seems, we need maintaining ISDN:
>     need fix bugs.
>     need not add new features.
>     need keep current features no touch.
> 
> 
>> No, we cannot be sure that a user gives us less than that. Your patch
>> just throws the rest to fix that overflow, right? What I'm saying I
>> wouldn't fix more than that.
> 
>   what you said is: this patch need improving, is it correct ?
>   if it is correct.
>     I still prefer to throw the rest contents (but need improving, too)
>       for maintaining, we need fix bug, but not need add new features
>       so what we should do:
>         a) fix the bug (should not memory overflow)
>         b) keep the original buffer length no touch
>            it is not sizeof(cmd.parm.cmsg.para) - 2)
>            it should be sizeof(cmd.param) - sizeof(cmd.param.cmsg) + sizeof(cmd.param.cmsg.para) - 2
>         c) need complete the relative document to export the limitation.
> 
>   is it ok ?

Yes, it is -- just fix the bug with minimal effort.

-- 
js
suse labs

Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow

[Chen Gang]

于 2013年03月05日 17:34, Jiri Slaby 写道:
>>   what you said is: this patch need improving, is it correct ?
>> >   if it is correct.
>> >     I still prefer to throw the rest contents (but need improving, too)
>> >       for maintaining, we need fix bug, but not need add new features
>> >       so what we should do:
>> >         a) fix the bug (should not memory overflow)
>> >         b) keep the original buffer length no touch
>> >            it is not sizeof(cmd.parm.cmsg.para) - 2)
>> >            it should be sizeof(cmd.param) - sizeof(cmd.param.cmsg) + sizeof(cmd.param.cmsg.para) - 2
>> >         c) need complete the relative document to export the limitation.
>> > 
>> >   is it ok ?
> Yes, it is -- just fix the bug with minimal effort.

  ok, I will send patch v2 for it.

  :-)

-- 
Chen Gang

Asianux Corporation

[PATCH v2] drivers/isdn: checkng length to be sure not memory overflow

[Chen Gang]

  sizeof (cmd.parm.cmsg.para) is 50 (MAX_CAPI_PARA_LEN).
  sizeof (cmd.parm) is 80+, but less than 100.
  strlen(msg) may be more than 80+ (Modem-Commandbuffer, less than 255).
    isdn_tty_send_msg is called by isdn_tty_parse_at
    the relative parameter is m->mdmcmd (atemu *m)
    the relative command may be "+M..."

  so need check the length to be sure not memory overflow.
    cmd.parm is a union, and need keep original valid buffer length no touch


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 drivers/isdn/i4l/isdn_tty.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
index d8a7d83..ebaebdf 100644
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c
@@ -902,7 +902,9 @@ isdn_tty_send_msg(modem_info *info, atemu *m, char *msg)
 	int j;
 	int l;
 
-	l = strlen(msg);
+	l = min(strlen(msg), sizeof(cmd.parm) - sizeof(cmd.parm.cmsg)
+		+ sizeof(cmd.parm.cmsg.para) - 2);
+
 	if (!l) {
 		isdn_tty_modem_result(RESULT_ERROR, info);
 		return;
-- 
1.7.7.6

Re: [PATCH v2] drivers/isdn: checkng length to be sure not memory overflow

[David Miller]

From: Chen Gang <gang.chen@asianux.com>
Date: Fri, 08 Mar 2013 12:25:41 +0800

> 
>   sizeof (cmd.parm.cmsg.para) is 50 (MAX_CAPI_PARA_LEN).
>   sizeof (cmd.parm) is 80+, but less than 100.
>   strlen(msg) may be more than 80+ (Modem-Commandbuffer, less than 255).
>     isdn_tty_send_msg is called by isdn_tty_parse_at
>     the relative parameter is m->mdmcmd (atemu *m)
>     the relative command may be "+M..."
> 
>   so need check the length to be sure not memory overflow.
>     cmd.parm is a union, and need keep original valid buffer length no touch
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>

Applied.