| Message ID | tencent_8D245D1A87D17619EA8475E8F005A151000A@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hugetlbfs: Fix integer overflow check in hugetlbfs_file_mmap() | expand |
… > To address this potential issue, this patch introduces a new check > that effectively prevents integer overflow. This check ensures the … Please choose an imperative change suggestion. See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc1#n94 Regards, Markus
On 10.07.23 10:32, Linke Li wrote: > From: Linke Li <lilinke99@gmail.com> > > 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) > return -EINVAL; > > The existing code includes an integer overflow check, which indicates > that the variable len has the potential to overflow, leading to undefined > behavior according to the C standard. However, both GCC and Clang > compilers may eliminate this overflow check based on the assumption > that there will be no undefined behavior. Although the Linux kernel > disables these optimizations by using the -fno-strict-overflow option, > there is still a risk if the compilers make mistakes in the future. So we're adding code to handle eventual future compiler bugs? That sounds wrong, but maybe I misunderstood the problem you are trying to solve?
On 07/11/23 13:49, David Hildenbrand wrote: > On 10.07.23 10:32, Linke Li wrote: > > From: Linke Li <lilinke99@gmail.com> > > > > 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) > > return -EINVAL; > > > > The existing code includes an integer overflow check, which indicates > > that the variable len has the potential to overflow, leading to undefined > > behavior according to the C standard. However, both GCC and Clang > > compilers may eliminate this overflow check based on the assumption > > that there will be no undefined behavior. Although the Linux kernel > > disables these optimizations by using the -fno-strict-overflow option, > > there is still a risk if the compilers make mistakes in the future. > > So we're adding code to handle eventual future compiler bugs? That sounds > wrong, but maybe I misunderstood the problem you are trying to solve? Like David, adding a fix for a potential future compiler bug sounds wrong. I have no problem with restructuring code to make it more immune to potential issues. However, it appears there are several places throughout the kernel that perform similar checks. For example: do_mmap() /* offset overflow? */ if ((pgoff + (len >> PAGE_SHIFT)) < pgoff) return -EOVERFLOW; expand_upwards() /* Enforce stack_guard_gap */ gap_addr = address + stack_guard_gap; /* Guard against overflow */ if (gap_addr < address || gap_addr > TASK_SIZE) gap_addr = TASK_SIZE; do_madvise() end = start + len; if (end < start) return -EINVAL; I am not suggesting that these all be changed. The question of a real issue still remains. 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.
> So we're adding code to handle eventual future compiler bugs? That sounds > wrong, but maybe I misunderstood the problem you are trying to solve? Sorry for not making it clear. My focus is the presence of undefined behavior in kernel code. Compilers can generate any code for undefined behavior and compiler developers will not take this as compiler bugs. In my option, kernel should not have undefined behavior. I double check this patch, this patch can not solve this issue well. I am considering a new patch below. The new patch do overflow check before the addition operation. ``` --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -155,10 +155,10 @@ 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 (vma_len > LLONG_MAX - ((loff_t)vma->vm_pgoff << PAGE_SHIFT)) return -EINVAL; + len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT); ```
> 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. Thank you for your valuable feedback.
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? 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);
On Thu, Jul 13, 2023 at 03:55:55PM +0800, linke li wrote: > > So we're adding code to handle eventual future compiler bugs? That sounds > > wrong, but maybe I misunderstood the problem you are trying to solve? > > Sorry for not making it clear. My focus is the presence of undefined > behavior in kernel code. > Compilers can generate any code for undefined behavior and compiler > developers will not > take this as compiler bugs. In my option, kernel should not have > undefined behavior. The point that several people have tried to make to you is that *this is not undefined behaviour*. The kernel is compiled with -fno-strict-overflow which causes the compiler to define signed arithmetic overflow to behave as twos-complement. Check the gcc documentation.
Thanks for your reply
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); >
> > 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. Thanks for Dan's advice. I have tested this macro in gcc-9 and clang-14, it can work well in both compilers and regardless of whether "-fno-strict-overflow" is added or not. I will send a new patch. Best Regards
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 7b17ccfa039d..1b4648a8e296 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -157,7 +157,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma) 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 (vma_len > LLONG_MAX - ((loff_t)vma->vm_pgoff << PAGE_SHIFT)) return -EINVAL; inode_lock(inode);