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 <[email protected]>
---
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
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
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