* [PATCH] hugetlbfs: fix protential null pointer dereference
@ 2019-04-10 2:50 Yufen Yu
2019-04-10 3:38 ` Mike Kravetz
0 siblings, 1 reply; 5+ messages in thread
From: Yufen Yu @ 2019-04-10 2:50 UTC (permalink / raw)
To: mike.kravetz, linux-mm; +Cc: kirill.shutemov, n-horiguchi, mhocko
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hugetlbfs: fix protential null pointer dereference
2019-04-10 2:50 [PATCH] hugetlbfs: fix protential null pointer dereference Yufen Yu
@ 2019-04-10 3:38 ` Mike Kravetz
2019-04-10 4:20 ` yuyufen
0 siblings, 1 reply; 5+ messages in thread
From: Mike Kravetz @ 2019-04-10 3:38 UTC (permalink / raw)
To: Yufen Yu, linux-mm; +Cc: kirill.shutemov, n-horiguchi, mhocko
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);
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hugetlbfs: fix protential null pointer dereference
2019-04-10 3:38 ` Mike Kravetz
@ 2019-04-10 4:20 ` yuyufen
2019-04-10 18:56 ` Mike Kravetz
0 siblings, 1 reply; 5+ messages in thread
From: yuyufen @ 2019-04-10 4:20 UTC (permalink / raw)
To: Mike Kravetz, linux-mm; +Cc: kirill.shutemov, n-horiguchi, mhocko, 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hugetlbfs: fix protential null pointer dereference
2019-04-10 4:20 ` yuyufen
@ 2019-04-10 18:56 ` Mike Kravetz
2019-04-11 3:30 ` yuyufen
0 siblings, 1 reply; 5+ messages in thread
From: Mike Kravetz @ 2019-04-10 18:56 UTC (permalink / raw)
To: yuyufen, linux-mm; +Cc: kirill.shutemov, n-horiguchi, mhocko
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hugetlbfs: fix protential null pointer dereference
2019-04-10 18:56 ` Mike Kravetz
@ 2019-04-11 3:30 ` yuyufen
0 siblings, 0 replies; 5+ messages in thread
From: yuyufen @ 2019-04-11 3:30 UTC (permalink / raw)
To: Mike Kravetz, linux-mm; +Cc: kirill.shutemov, n-horiguchi, mhocko
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-11 3:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-10 2:50 [PATCH] hugetlbfs: fix protential null pointer dereference Yufen Yu
2019-04-10 3:38 ` Mike Kravetz
2019-04-10 4:20 ` yuyufen
2019-04-10 18:56 ` Mike Kravetz
2019-04-11 3:30 ` yuyufen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.