On Fri, Dec 20, 2013 at 06:19:56PM +0800, Wenliang Fan wrote: > The checking condition in 'validateFlash2xReadWrite()' is not > sufficient. A large number invalid would cause an integer overflow and > pass the condition, which could cause further integer overflows in > 'Bcmchar.c:bcm_char_ioctl()'. > > Signed-off-by: Wenliang Fan <fanwlexca@xxxxxxxxx> > --- > drivers/staging/bcm/nvm.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/staging/bcm/nvm.c b/drivers/staging/bcm/nvm.c > index 9e5f955..7f3dd4b 100644 > --- a/drivers/staging/bcm/nvm.c > +++ b/drivers/staging/bcm/nvm.c > @@ -3944,6 +3944,11 @@ int validateFlash2xReadWrite(struct bcm_mini_adapter *Adapter, struct bcm_flash2 > > BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, NVM_RW, DBG_LVL_ALL, "End offset :%x\n", uiSectEndOffset); > > + /* psFlash2xReadWrite->offset and uiNumOfBytes are user controlled and can lead to integer overflows */ > + if (uiSectStartOffset + psFlash2xReadWrite->offset < uiSectStartOffset) > + return false; > + if (uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes < uiNumOfBytes) > + return false; Please don't do this... :( Just do it exactly like I explained before. Using this style of overflow checking causes static checkers which look for integer overflows to complain and it is too complicated. Just do: if (uiSectStartOffset > uiSectEndOffset) return false; if (psFlash2xReadWrite->offset > uiSectEndOffset) return false; > /* Checking the boundary condition */ > if ((uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes) <= uiSectEndOffset) > return TRUE; Reverse this condition so it does: if (uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes > uiSectEndOffset) return false; Then if everything is valid do: return true; Compare how these two statements sound in English: If psFlash2xReadWrite->offset is larger than uiSectEndOffset return false. Vs: If we uiSectStartOffset plus psFlash2xReadWrite->offset plus uiNumOfBytes is less than uiNumOfBytes then it means we overflowed. Since we verified that uiSectStartOffset plus psFlash2xReadWrite->offset don't overflow that means uiNumOfBytes is too large so return false. The first one is simple and the second one is complicated. Don't do complicated things for no reason. Also the first one is fewer operations for the CPU. regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
