Re: [bug report] hugetlbfs: fix offset overflow in hugetlbfs mmap

[Mike Kravetz <mike.kravetz@oracle.com>]

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@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>