* [PATCH] x86: Fix possible null-pointer dereferences in untrack_pfn()
@ 2019-07-23 13:26 Jia-Ju Bai
2019-07-24 11:16 ` Thomas Gleixner
0 siblings, 1 reply; 3+ messages in thread
From: Jia-Ju Bai @ 2019-07-23 13:26 UTC (permalink / raw)
To: dave.hansen, luto, peterz, tglx, mingo, bp, hpa
Cc: x86, linux-kernel, Jia-Ju Bai
In untrack_pfn(), there is an if statement on line 1058 to check whether
vma is NULL:
if (vma && !(vma->vm_flags & VM_PAT))
When vma is NULL, vma is used on line 1064:
if (follow_phys(vma, vma->vm_start, 0, &prot, &paddr))
and line 1069:
size = vma->vm_end - vma->vm_start;
Thus, possible null-pointer dereferences may occur.
To fix these possible bugs, vma is checked on line 1063.
These bugs are found by a static analysis tool STCheck written by us.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
arch/x86/mm/pat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index d9fbd4f69920..717456e7745e 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -1060,7 +1060,7 @@ void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
/* free the chunk starting from pfn or the whole chunk */
paddr = (resource_size_t)pfn << PAGE_SHIFT;
- if (!paddr && !size) {
+ if (vma && !paddr && !size) {
if (follow_phys(vma, vma->vm_start, 0, &prot, &paddr)) {
WARN_ON_ONCE(1);
return;
--
2.17.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] x86: Fix possible null-pointer dereferences in untrack_pfn()
2019-07-23 13:26 [PATCH] x86: Fix possible null-pointer dereferences in untrack_pfn() Jia-Ju Bai
@ 2019-07-24 11:16 ` Thomas Gleixner
2019-07-24 11:34 ` Thomas Gleixner
0 siblings, 1 reply; 3+ messages in thread
From: Thomas Gleixner @ 2019-07-24 11:16 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: dave.hansen, luto, peterz, mingo, bp, hpa, x86, linux-kernel
On Tue, 23 Jul 2019, Jia-Ju Bai wrote:
> In untrack_pfn(), there is an if statement on line 1058 to check whether
> vma is NULL:
> if (vma && !(vma->vm_flags & VM_PAT))
>
> When vma is NULL, vma is used on line 1064:
> if (follow_phys(vma, vma->vm_start, 0, &prot, &paddr))
> and line 1069:
> size = vma->vm_end - vma->vm_start;
>
> Thus, possible null-pointer dereferences may occur.
>
> To fix these possible bugs, vma is checked on line 1063.
>
> These bugs are found by a static analysis tool STCheck written by us.
In principle you are right, but that's a bit more subtle as the callers can
provide a vma pointer and/or a valid pfn and size.
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index d9fbd4f69920..717456e7745e 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -1060,7 +1060,7 @@ void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
>
> /* free the chunk starting from pfn or the whole chunk */
> paddr = (resource_size_t)pfn << PAGE_SHIFT;
> - if (!paddr && !size) {
> + if (vma && !paddr && !size) {
> if (follow_phys(vma, vma->vm_start, 0, &prot, &paddr)) {
> WARN_ON_ONCE(1);
> return;
So I'd rather have a sanity check in that function which does:
if (WARN_ON_ONCE(!vma && !pfn && !size))
return;
Thanks,
tglx
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] x86: Fix possible null-pointer dereferences in untrack_pfn()
2019-07-24 11:16 ` Thomas Gleixner
@ 2019-07-24 11:34 ` Thomas Gleixner
0 siblings, 0 replies; 3+ messages in thread
From: Thomas Gleixner @ 2019-07-24 11:34 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: dave.hansen, luto, peterz, mingo, bp, hpa, x86, linux-kernel
On Wed, 24 Jul 2019, Thomas Gleixner wrote:
> On Tue, 23 Jul 2019, Jia-Ju Bai wrote:
>
> > In untrack_pfn(), there is an if statement on line 1058 to check whether
> > vma is NULL:
> > if (vma && !(vma->vm_flags & VM_PAT))
> >
> > When vma is NULL, vma is used on line 1064:
> > if (follow_phys(vma, vma->vm_start, 0, &prot, &paddr))
> > and line 1069:
> > size = vma->vm_end - vma->vm_start;
> >
> > Thus, possible null-pointer dereferences may occur.
> >
> > To fix these possible bugs, vma is checked on line 1063.
> >
> > These bugs are found by a static analysis tool STCheck written by us.
>
> In principle you are right, but that's a bit more subtle as the callers can
> provide a vma pointer and/or a valid pfn and size.
>
> > diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> > index d9fbd4f69920..717456e7745e 100644
> > --- a/arch/x86/mm/pat.c
> > +++ b/arch/x86/mm/pat.c
> > @@ -1060,7 +1060,7 @@ void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
> >
> > /* free the chunk starting from pfn or the whole chunk */
> > paddr = (resource_size_t)pfn << PAGE_SHIFT;
> > - if (!paddr && !size) {
> > + if (vma && !paddr && !size) {
> > if (follow_phys(vma, vma->vm_start, 0, &prot, &paddr)) {
> > WARN_ON_ONCE(1);
> > return;
>
> So I'd rather have a sanity check in that function which does:
>
> if (WARN_ON_ONCE(!vma && !pfn && !size))
> return;
The even better solution is to have separate functions:
untrack_pfn(unsigned long pfn, unsigned long size)
and
untrack_vma(struct vm_area_struct *vma, unsigned long pfn, unsigned long size)
The amount of shared code is minimal and the result is less confusing.
Thanks,
tglx
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-07-24 11:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-23 13:26 [PATCH] x86: Fix possible null-pointer dereferences in untrack_pfn() Jia-Ju Bai
2019-07-24 11:16 ` Thomas Gleixner
2019-07-24 11:34 ` Thomas Gleixner
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.