* [PATCH v2] Staging: bcm: Fix potential buffer overflow and style
@ 2011-01-24 1:12 Javier Martinez Canillas
2011-01-24 5:05 ` [PATCH v2] Staging: bcm: Fix potential buffer overflow and Dan Carpenter
0 siblings, 1 reply; 2+ messages in thread
From: Javier Martinez Canillas @ 2011-01-24 1:12 UTC (permalink / raw)
To: kernel-janitors
bcm driver copies from userpace with the following statement:
copy_from_user(&RxCntrlMsgBitMask, IoBuffer.InputBuffer, IoBuffer.InputLength);
Compiling gives the following warning:
In function ‘copy_from_user’,
inlined from ‘bcm_char_ioctl’ at drivers/staging/bcm/Bcmchar.c:2030:
linux-next/arch/x86/include/asm/uaccess_32.h:212: warning: call to
‘copy_from_user_overflow’ declared with attribute warning: copy_from_user()
buffer size is not provably correct.
RxCntrlMsgBitMask is of type unsigned long so we have to check that
IoBuffer.InputLength is not greater than sizeof(unsigned long).
Signed-off-by: Javier Martinez Canillas <martinez.javier@gmail.com>
---
V2: Check the user provided length recommended by Stephen Hemminger
Check for copy_from_user() and return -EFAULT; do not use ULONG and
get rid of unnecessary log information recommended by Dan Carpenter.
drivers/staging/bcm/Bcmchar.c | 48 +++++++++++++++++++++++-----------------
1 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/drivers/staging/bcm/Bcmchar.c b/drivers/staging/bcm/Bcmchar.c
index 31674ea..969c766 100644
--- a/drivers/staging/bcm/Bcmchar.c
+++ b/drivers/staging/bcm/Bcmchar.c
@@ -2015,28 +2015,36 @@ static long bcm_char_ioctl(struct file *filp, UINT cmd, ULONG arg)
break ;
}
- case IOCTL_BCM_CNTRLMSG_MASK:
- {
- ULONG RxCntrlMsgBitMask = 0 ;
+ case IOCTL_BCM_CNTRLMSG_MASK:
+ {
+ unsigned long RxCntrlMsgBitMask = 0;
+
+ /* Copy Ioctl Buffer structure */
+ Status = copy_from_user(&IoBuffer, argp, sizeof(IOCTL_BUFFER));
+ if (Status) {
+ BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,
+ "copy of Ioctl buffer is failed from user space");
+ Status = -EFAULT;
+ break;
+ }
- /* Copy Ioctl Buffer structure */
- Status = copy_from_user(&IoBuffer, argp, sizeof(IOCTL_BUFFER));
- if(Status)
- {
- BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,"copy of Ioctl buffer is failed from user space");
- break;
- }
+ if (IoBuffer.InputLength > sizeof(unsigned long)) {
+ Status = -EINVAL;
+ break;
+ }
- Status = copy_from_user(&RxCntrlMsgBitMask, IoBuffer.InputBuffer, IoBuffer.InputLength);
- if(Status)
- {
- BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,"copy of control bit mask failed from user space");
- break;
- }
- BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,"\n Got user defined cntrl msg bit mask :%lx", RxCntrlMsgBitMask);
- pTarang->RxCntrlMsgBitMask = RxCntrlMsgBitMask ;
- }
- break;
+ Status = copy_from_user(&RxCntrlMsgBitMask, IoBuffer.InputBuffer,
+ IoBuffer.InputLength);
+ if (Status) {
+ BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,
+ "copy of control bit mask failed from user space");
+ Status = -EFAULT;
+ break;
+ }
+
+ pTarang->RxCntrlMsgBitMask = RxCntrlMsgBitMask;
+ }
+ break;
case IOCTL_BCM_GET_DEVICE_DRIVER_INFO:
{
DEVICE_DRIVER_INFO DevInfo;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH v2] Staging: bcm: Fix potential buffer overflow and
2011-01-24 1:12 [PATCH v2] Staging: bcm: Fix potential buffer overflow and style Javier Martinez Canillas
@ 2011-01-24 5:05 ` Dan Carpenter
0 siblings, 0 replies; 2+ messages in thread
From: Dan Carpenter @ 2011-01-24 5:05 UTC (permalink / raw)
To: kernel-janitors
On Mon, Jan 24, 2011 at 02:12:28AM +0100, Javier Martinez Canillas wrote:
> + case IOCTL_BCM_CNTRLMSG_MASK:
> + {
This should be indented two tabs. One for the function and one for the
switch statement.
> + unsigned long RxCntrlMsgBitMask = 0;
> +
> + /* Copy Ioctl Buffer structure */
> + Status = copy_from_user(&IoBuffer, argp, sizeof(IOCTL_BUFFER));
> + if (Status) {
> + BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,
> + "copy of Ioctl buffer is failed from user space");
> + Status = -EFAULT;
> + break;
> + }
>
> - /* Copy Ioctl Buffer structure */
> - Status = copy_from_user(&IoBuffer, argp, sizeof(IOCTL_BUFFER));
> - if(Status)
> - {
> - BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,"copy of Ioctl buffer is failed from user space");
> - break;
> - }
> + if (IoBuffer.InputLength > sizeof(unsigned long)) {
Personally I would say:
if (IoBuffer.InputLength != sizeof(unsigned long)) {
There is no other size that makes sense here. It's more helpful to
return an error directly.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2011-01-24 5:05 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-01-24 1:12 [PATCH v2] Staging: bcm: Fix potential buffer overflow and style Javier Martinez Canillas
2011-01-24 5:05 ` [PATCH v2] Staging: bcm: Fix potential buffer overflow and Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.