| Message ID | 512DCC4A.6060106@asianux.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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,
于 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.
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" :).
于 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. :-)
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.)
于 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.
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.
于 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. :-)
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.
于 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. :-)
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;
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(-)