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

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

[Dan Carpenter]

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.

   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>

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

[Mike Kravetz]

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>

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

[Dan Carpenter]

I only send new warnings, but in this case it doesn't find any similar
bugs in hugetlbfs.  It's specifically looking for code like
"(foo + bar < foo)" where the addition operation is signed.

Could you CC me on the fixes, just for reference and maybe I can check
for those as well.

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>