Patchwork drivers/isdn: checkng length to be sure not memory overflow

login
register
mail settings
Submitter Chen Gang
Date Feb. 27, 2013, 9:05 a.m.
Message ID <512DCC4A.6060106@asianux.com>
Download mbox | patch
Permalink /patch/223560/
State Rejected
Delegated to: David Miller
Headers show

Comments

Chen Gang - Feb. 27, 2013, 9:05 a.m.
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(-)
Jiri Slaby - Feb. 27, 2013, 9:48 a.m.
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,
Chen Gang - Feb. 27, 2013, 10:25 a.m.
于 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.
Jiri Slaby - Feb. 27, 2013, 10:44 a.m.
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" :).
Chen Gang - Feb. 28, 2013, 1:45 a.m.
于 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.

  :-)
Jiri Slaby - Feb. 28, 2013, 10 a.m.
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.)
Chen Gang - Feb. 28, 2013, 11:01 a.m.
于 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.
Jiri Slaby - Feb. 28, 2013, 1:43 p.m.
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.
Chen Gang - March 5, 2013, 2:20 a.m.
于 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.

  :-)
Jiri Slaby - March 5, 2013, 9:34 a.m.
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.
Chen Gang - March 7, 2013, 4:13 a.m.
于 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.

  :-)

Patch

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;