[PATCH] hugetlbfs: fix protential null pointer dereference

[PATCH] hugetlbfs: fix protential null pointer dereference

[Yufen Yu]

After commit 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map"),
i_mapping->private_data will be NULL for mode that is not regular and link.
Then, it might cause NULL pointer derefernce in hugetlb_reserve_pages()
when do_mmap. We can avoid protential null pointer dereference by
judging whether it have been allocated.

Fixes: 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map")
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Michal Hocko <mhocko@kernel.org>
Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 mm/hugetlb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 97b1e0290c66..15e4baf2aa7d 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4465,6 +4465,8 @@ int hugetlb_reserve_pages(struct inode *inode,
 	 */
 	if (!vma || vma->vm_flags & VM_MAYSHARE) {
 		resv_map = inode_resv_map(inode);
+		if (!resv_map)
+			return -EOPNOTSUPP;
 
 		chg = region_chg(resv_map, from, to);
 
-- 
2.16.2.dirty

Re: [PATCH] hugetlbfs: fix protential null pointer dereference

[Mike Kravetz]

On 4/9/19 7:50 PM, Yufen Yu wrote:
> After commit 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map"),
> i_mapping->private_data will be NULL for mode that is not regular and link.
> Then, it might cause NULL pointer derefernce in hugetlb_reserve_pages()
> when do_mmap. We can avoid protential null pointer dereference by
> judging whether it have been allocated.
> 
> Fixes: 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map")
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>

Thanks for catching this.  I mistakenly thought all the code was checking
for NULL resv_map.  That certainly is one (and only) place where it is not
checked.  Have you verified that this is possible?  Should be pretty easy
to do.  If you have not, I can try to verify tomorrow.

> ---
>  mm/hugetlb.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 97b1e0290c66..15e4baf2aa7d 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4465,6 +4465,8 @@ int hugetlb_reserve_pages(struct inode *inode,
>  	 */
>  	if (!vma || vma->vm_flags & VM_MAYSHARE) {
>  		resv_map = inode_resv_map(inode);
> +		if (!resv_map)
> +			return -EOPNOTSUPP;

I'm not sure about the return code here.  Note that all callers of
hugetlb_reserve_pages() force return value of -ENOMEM if non-zero value
is returned.  I think we would like to return -EACCES in this situation.
The mmap man page says:

       EACCES A  file descriptor refers to a non-regular file.  Or ...
-- 
Mike Kravetz

>  
>  		chg = region_chg(resv_map, from, to);
>  
> 

Re: [PATCH] hugetlbfs: fix protential null pointer dereference

[yuyufen]

Hi, Mike


On 2019/4/10 11:38, Mike Kravetz wrote:
> On 4/9/19 7:50 PM, Yufen Yu wrote:
>> After commit 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map"),
>> i_mapping->private_data will be NULL for mode that is not regular and link.
>> Then, it might cause NULL pointer derefernce in hugetlb_reserve_pages()
>> when do_mmap. We can avoid protential null pointer dereference by
>> judging whether it have been allocated.
>>
>> Fixes: 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map")
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> Thanks for catching this.  I mistakenly thought all the code was checking
> for NULL resv_map.  That certainly is one (and only) place where it is not
> checked.  Have you verified that this is possible?  Should be pretty easy
> to do.  If you have not, I can try to verify tomorrow.

I honestly say that I don't have verified.

>> ---
>>   mm/hugetlb.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 97b1e0290c66..15e4baf2aa7d 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -4465,6 +4465,8 @@ int hugetlb_reserve_pages(struct inode *inode,
>>   	 */
>>   	if (!vma || vma->vm_flags & VM_MAYSHARE) {
>>   		resv_map = inode_resv_map(inode);
>> +		if (!resv_map)
>> +			return -EOPNOTSUPP;
> I'm not sure about the return code here.  Note that all callers of
> hugetlb_reserve_pages() force return value of -ENOMEM if non-zero value
> is returned.  I think we would like to return -EACCES in this situation.
> The mmap man page says:
>
>         EACCES A  file descriptor refers to a non-regular file.  Or ...

Thanks for your suggestion. It is more reasonable to use -EACCES.

Yufen
Thanks.

Re: [PATCH] hugetlbfs: fix protential null pointer dereference

[Mike Kravetz]

On 4/9/19 9:20 PM, yuyufen wrote:
> Hi, Mike
> 
> On 2019/4/10 11:38, Mike Kravetz wrote:
>> On 4/9/19 7:50 PM, Yufen Yu wrote:
>>> After commit 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map"),
>>> i_mapping->private_data will be NULL for mode that is not regular and link.
>>> Then, it might cause NULL pointer derefernce in hugetlb_reserve_pages()
>>> when do_mmap. We can avoid protential null pointer dereference by
>>> judging whether it have been allocated.
>>>
>>> Fixes: 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map")
>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>> Cc: Michal Hocko <mhocko@kernel.org>
>>> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
>> Thanks for catching this.  I mistakenly thought all the code was checking
>> for NULL resv_map.  That certainly is one (and only) place where it is not
>> checked.  Have you verified that this is possible?  Should be pretty easy
>> to do.  If you have not, I can try to verify tomorrow.
> 
> I honestly say that I don't have verified.

I do not believe it is possible to hit this condition in the existing code.
Why?  hugetlb_reserve_pages is only called from two places:
1) hugetlb_file_setup. In this case the inode is created immediately before
   the call with S_IFREG.  Hence a regular file so resv_map created.
2) hugetlbfs_file_mmap called via do_mmap.  In do_mmap, there is the following
   check:
        if (!file->f_op->mmap)
                return -ENODEV;
   In the hugetlbfs inode creation code (hugetlbfs_get_inode), note that
   inode->i_fop = &hugetlbfs_file_operations (containing hugetlbfs_file_mmap)
   is only set for inodes of type S_IFREG.  And, resv_map are created
   for these.  So, mmap will not call hugetlbfs_file_mmap for non-S_IFREG
   hugetlbfs inode.  Instead, it will return ENODEV.

Even if we can not hit this condition today, I still believe it would be
a good idea to make this type of change.  It would prevent a possible NULL
dereference in case the structure of code changes in the future.  However,
unless I am mistaken this is not needed as an urgent fix.

-- 
Mike Kravetz

Re: [PATCH] hugetlbfs: fix protential null pointer dereference

[yuyufen]

On 2019/4/11 2:56, Mike Kravetz wrote:
> On 4/9/19 9:20 PM, yuyufen wrote:
>> Hi, Mike
>>
>> On 2019/4/10 11:38, Mike Kravetz wrote:
>>> On 4/9/19 7:50 PM, Yufen Yu wrote:
>>>> After commit 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map"),
>>>> i_mapping->private_data will be NULL for mode that is not regular and link.
>>>> Then, it might cause NULL pointer derefernce in hugetlb_reserve_pages()
>>>> when do_mmap. We can avoid protential null pointer dereference by
>>>> judging whether it have been allocated.
>>>>
>>>> Fixes: 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map")
>>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>>> Cc: Michal Hocko <mhocko@kernel.org>
>>>> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
>>> Thanks for catching this.  I mistakenly thought all the code was checking
>>> for NULL resv_map.  That certainly is one (and only) place where it is not
>>> checked.  Have you verified that this is possible?  Should be pretty easy
>>> to do.  If you have not, I can try to verify tomorrow.
>> I honestly say that I don't have verified.
> I do not believe it is possible to hit this condition in the existing code.
> Why?  hugetlb_reserve_pages is only called from two places:
> 1) hugetlb_file_setup. In this case the inode is created immediately before
>     the call with S_IFREG.  Hence a regular file so resv_map created.
> 2) hugetlbfs_file_mmap called via do_mmap.  In do_mmap, there is the following
>     check:
>          if (!file->f_op->mmap)
>                  return -ENODEV;
>     In the hugetlbfs inode creation code (hugetlbfs_get_inode), note that
>     inode->i_fop = &hugetlbfs_file_operations (containing hugetlbfs_file_mmap)
>     is only set for inodes of type S_IFREG.  And, resv_map are created
>     for these.  So, mmap will not call hugetlbfs_file_mmap for non-S_IFREG
>     hugetlbfs inode.  Instead, it will return ENODEV.
>
> Even if we can not hit this condition today, I still believe it would be
> a good idea to make this type of change.  It would prevent a possible NULL
> dereference in case the structure of code changes in the future.  However,
> unless I am mistaken this is not needed as an urgent fix.

Thanks for so much detailed explanation. I will resend v2 including 
these suggestion.