[v2] hugetlbfs: Fix integer overflow check in hugetlbfs_file_mmap()

Commit Message

Linke Li July 20, 2023, 1:49 p.m. UTC

From: Linke Li <[e-mail]>

```
	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;
```

There is a signed integer overflow in the code, which is undefined
behavior according to the C stacnard. Although kernel disables some
optimizations by using the "-fno-strict-overflow" option, there is
still a risk.

Using macro "check_add_overflow" to do the overflow check can
effectively detect integer overflow and avoid any undefined behavior.

Signed-off-by: Linke Li <[e-mail]>
---
 fs/hugetlbfs/inode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Dan Carpenter July 20, 2023, 2:13 p.m. UTC | #1

On Thu, Jul 20, 2023 at 09:49:39PM +0800, Linke Li wrote:
> From: Linke Li <[e-mail]>
> 
> ```
> 	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;
> ```
> 
> There is a signed integer overflow in the code, which is undefined
> behavior according to the C stacnard. Although kernel disables some
> optimizations by using the "-fno-strict-overflow" option, there is
> still a risk.

It's not a risk.  Better to say, "although this works, it's still a bit
ugly and static checkers will complain".

I wouldn't have commented on the commit message except that this patch
checkpatch warning so you're going to have to redo it anyway.  Run
scripts/checkpatch.pl on your patches before sending them.

WARNING: please, no spaces at the start of a line
#49: FILE: fs/hugetlbfs/inode.c:158:
+    if (check_add_overflow(vma_len, (loff_t)vma->vm_pgoff << PAGE_SHIFT, &len))$

WARNING: suspect code indent for conditional statements (4, 16)
#49: FILE: fs/hugetlbfs/inode.c:158:
+    if (check_add_overflow(vma_len, (loff_t)vma->vm_pgoff << PAGE_SHIFT, &len))
                return -EINVAL;

total: 0 errors, 2 warnings, 10 lines checked

regards,
dan carpenter

Patch

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 7b17ccfa039d..60f3010b0f71 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -155,9 +155,7 @@  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);