On 07/13/23 18:10, Dan Carpenter wrote: > On Thu, Jul 13, 2023 at 03:57:00PM +0800, linke li wrote: > > > However, if this is a real issue it would make more > > > sense to look for and change all such checks rather than one single occurrence. > > > > Hi, Mike. I have checked the example code you provided, and the > > difference between > > those codes and the patched code is that those checks are checks for > > unsigned integer > > overflow, which is well-defined. Only undefined behavior poses a > > security risk. So they > > don't need any modifications. I have only found one occurrence of > > signed number > > overflow so far. > > I used to have a similar check to that but I eventually deleted it > because I decided that the -fno-strict-overflow option works. It didn't > produce a lot of warnings. > > Historically we have done a bad job at open coding integer overflow > checks. Some that I wrote turned out to be incorrect. And even when > I write them correctly a couple times people have "fixed" them even > harder without CCing me or asking me why I wrote them the way I did. > > What about using the check_add_overflow() macro? I like the macro. It seems to have plenty of users. Linke Li, what do you think? If you like, please send another path using the macro as suggested by Dan. > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index 7b17ccfa039d..c512165736e0 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -155,9 +155,8 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma) > return -EINVAL; > > vma_len = (loff_t)(vma->vm_end - vma->vm_start); > - len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT); > - /* check for overflow */ > - if (len < vma_len) > + if (check_add_overflow(vma_len, (loff_t)vma->vm_pgoff << PAGE_SHIFT, > + &len)) > return -EINVAL; > > inode_lock(inode); > -- Mike Kravetz
