On 04/21/2017 03:57 AM, Dan Carpenter wrote:
> Hello Mike Kravetz,
>
> The patch 045c7a3f53d9: "hugetlbfs: fix offset overflow in hugetlbfs
> mmap" from Apr 13, 2017, leads to the following static checker
> warning:
>
> fs/hugetlbfs/inode.c:152 hugetlbfs_file_mmap()
> warn: signed overflow undefined. 'vma_len + (vma->vm_pgoff << 12) < vma_len'
>
> fs/hugetlbfs/inode.c
> 121 static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
> 122 {
> 123 struct inode *inode = file_inode(file);
> 124 loff_t len, vma_len;
> 125 int ret;
> 126 struct hstate *h = hstate_file(file);
> 127
> 128 /*
> 129 * vma address alignment (but not the pgoff alignment) has
> 130 * already been checked by prepare_hugepage_range. If you add
> 131 * any error returns here, do so after setting VM_HUGETLB, so
> 132 * is_vm_hugetlb_page tests below unmap_region go the right
> 133 * way when do_mmap_pgoff unwinds (may be important on powerpc
> 134 * and ia64).
> 135 */
> 136 vma->vm_flags |= VM_HUGETLB | VM_DONTEXPAND;
> 137 vma->vm_ops = &hugetlb_vm_ops;
> 138
> 139 /*
> 140 * Offset passed to mmap (before page shift) could have been
> 141 * negative when represented as a (l)off_t.
> 142 */
> 143 if (((loff_t)vma->vm_pgoff << PAGE_SHIFT) < 0)
> 144 return -EINVAL;
> 145
> 146 if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
> 147 return -EINVAL;
> 148
> 149 vma_len = (loff_t)(vma->vm_end - vma->vm_start);
> 150 len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
> 151 /* check for overflow */
> 152 if (len < vma_len)
> ^^^^^^^^^^^^^
> This is undefined in C. I think with kernel GCC options it's safe these
> days, but I can't swear on it.
>
Thanks Dan,
We discussed this a bit when the patch was submitted. I'm just curious
if your static checker is checking all code or just new patches? The
reason for asking is that there are similar issues later on in this
routine that ware not changed by this patch. In fact, there are several
instances of this type of 'undefined behavior' in the hugetlbfs code.
I can go through the code and try to change all such instances. However,
by code inspection alone I am likely to miss some. If you or others can
point out a tool (or compiler options) to look for all such instances that
would make such an exercise easier.
--
Mike Kravetz
> 153 return -EINVAL;
> 154
> 155 inode_lock(inode);
> 156 file_accessed(file);
>
> regards,
> dan carpenter
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
