From: Linke Li <[email protected]>
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.
To address this potential issue, this patch introduces a new check
that effectively prevents integer overflow. This check ensures the
safe operation of the code even when the Linux kernel is compiled
without the -fno-strict-overflow option, providing an added layer
of protection against potential issues caused by compiler behavior.
Signed-off-by: Linke Li <[email protected]>
---
fs/hugetlbfs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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);
--
2.25.1
On 10.07.23 10:32, Linke Li wrote:
> From: Linke Li <[email protected]>
>
> 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?
--
Cheers,
David / dhildenb
On 07/11/23 13:49, David Hildenbrand wrote:
> On 10.07.23 10:32, Linke Li wrote:
> > From: Linke Li <[email protected]>
> >
> > 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.
--
Mike Kravetz
> 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);
>
--
Mike Kravetz
> > 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