| Message ID | 20190410025037.144872-1-yuyufen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hugetlbfs: fix protential null pointer dereference | expand |
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 ...
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.
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.
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.
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);
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(+)