Hello Chen Gang,
The patch f39479363e03: "drivers/isdn: checkng length to be sure not
memory overflow" from Mar 7, 2013, leads to the following static checker
warning:
"drivers/isdn/i4l/isdn_tty.c:969 isdn_tty_send_msg()
error: buffer overflow 'cmd.parm.cmsg.para' 50 <= 73"
drivers/isdn/i4l/isdn_tty.c
905 l = min(strlen(msg), sizeof(cmd.parm) - sizeof(cmd.parm.cmsg)
906 + sizeof(cmd.parm.cmsg.para) - 2);
907
[ snip ]
963 cmd.parm.cmsg.Length = l + 14;
964 cmd.parm.cmsg.Command = CAPI_MANUFACTURER;
965 cmd.parm.cmsg.Subcommand = CAPI_REQ;
966 cmd.parm.cmsg.adr.Controller = info->isdn_driver + 1;
967 cmd.parm.cmsg.para[0] = l + 1;
968 strncpy(&cmd.parm.cmsg.para[1], msg, l);
969 cmd.parm.cmsg.para[l + 1] = 0xd;
^^^^^^^
"l" is more than sizeof(cmd.parm.cmsg.para) here so it is an overflow.
As far as I can see the correct limit should be:
l = min(strlen(msg), sizeof(cmd.parm.cmsg.para) - 2);
The "- 2" is so that ".cmsg.para[l + 1] = 0xd" does not overflow.
regards,
dan carpenter
On 05/23/2013 05:05 AM, Dan Carpenter wrote:
The patch f39479363e03: "drivers/isdn: checkng length to be sure not
memory overflow" from Mar 7, 2013, leads to the following static checker
warning:
"drivers/isdn/i4l/isdn_tty.c:969 isdn_tty_send_msg()
error: buffer overflow 'cmd.parm.cmsg.para' 50 <= 73"
drivers/isdn/i4l/isdn_tty.c
905 l = min(strlen(msg), sizeof(cmd.parm) - sizeof(cmd.parm.cmsg)
906 + sizeof(cmd.parm.cmsg.para) - 2);
907
[ snip ]
963 cmd.parm.cmsg.Length = l + 14;
964 cmd.parm.cmsg.Command = CAPI_MANUFACTURER;
965 cmd.parm.cmsg.Subcommand = CAPI_REQ;
966 cmd.parm.cmsg.adr.Controller = info->isdn_driver + 1;
967 cmd.parm.cmsg.para[0] = l + 1;
968 strncpy(&cmd.parm.cmsg.para[1], msg, l);
969 cmd.parm.cmsg.para[l + 1] = 0xd;
^^^^^^^
"l" is more than sizeof(cmd.parm.cmsg.para) here so it is an overflow.
As far as I can see the correct limit should be:
l = min(strlen(msg), sizeof(cmd.parm.cmsg.para) - 2);
The "- 2" is so that ".cmsg.para[l + 1] = 0xd" does not overflow.
'cmd.parm' is a union in "include/linux/isdnif.h", so if length is more
than 'sizeof(cmd.parm.cmsg.para) - 2', but still within the 'cmd.parm',
it will be still OK (can not cause memory over flow).
The patch tries to keep original API no touch, and only fix the bug
(try not to decrease the original real valid buffer length).
Excuse me, the patch comments is not with standard format, so may cause
the readers misunderstanding.
Thanks.
------------------------------comments begin--------------------------------------
commit f39479363e0361c8bb4397481c01a7c3a1a3c8ac
Author: Chen Gang [off-list ref]
Date: Thu Mar 7 18:25:41 2013 +0000
drivers/isdn: checkng length to be sure not memory overflow
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 [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
------------------------------comments end--------------------------------------
--
Chen Gang
Asianux Corporation