LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Huge TLB: Potential NULL deref in arch/x86/mm/hugetlbpage.c:huge_pmd_share()
@ 2011-02-03 20:50 Jesper Juhl
  2011-02-03 21:01 ` Andrew Morton
  0 siblings, 1 reply; 5+ messages in thread
From: Jesper Juhl @ 2011-02-03 20:50 UTC (permalink / raw)
  To: linux-kernel
  Cc: x86, linux-mm, Tejun Heo, H. Peter Anvin, Ingo Molnar,
	Thomas Gleixner, Rohit Seth, Andrew Morton

In arch/x86/mm/hugetlbpage.c:huge_pmd_share() we call find_vma(mm, addr) 
and then subsequently dereference the pointer returned without checking if 
it was NULL. I can't find anything that guarantees that find_vma() will 
never return NULL in this case, so I believe there's a genuine bug. 
However, I'd greatly appreciate it if someone would take the time to 
double check me.
This patch implements what I believe to be the correct handling of a NULL 
return from find_vma(). Please consider for inclusion.

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 hugetlbpage.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
index 069ce7c..0ebd3d0 100644
--- a/arch/x86/mm/hugetlbpage.c
+++ b/arch/x86/mm/hugetlbpage.c
@@ -61,14 +61,19 @@ static int vma_shareable(struct vm_area_struct *vma, unsigned long addr)
 static void huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
 {
 	struct vm_area_struct *vma = find_vma(mm, addr);
-	struct address_space *mapping = vma->vm_file->f_mapping;
-	pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
-			vma->vm_pgoff;
+	struct address_space *mapping;
+	pgoff_t idx;
 	struct prio_tree_iter iter;
 	struct vm_area_struct *svma;
 	unsigned long saddr;
 	pte_t *spte = NULL;
 
+	if (!vma)
+		return;
+
+	mapping = vma->vm_file->f_mapping;
+	idx = ((addr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
+
 	if (!vma_shareable(vma, addr))
 		return;
 

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Huge TLB: Potential NULL deref in arch/x86/mm/hugetlbpage.c:huge_pmd_share()
  2011-02-03 20:50 [PATCH] Huge TLB: Potential NULL deref in arch/x86/mm/hugetlbpage.c:huge_pmd_share() Jesper Juhl
@ 2011-02-03 21:01 ` Andrew Morton
  2011-02-03 21:17   ` Jesper Juhl
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2011-02-03 21:01 UTC (permalink / raw)
  To: Jesper Juhl
  Cc: linux-kernel, x86, linux-mm, Tejun Heo, H. Peter Anvin,
	Ingo Molnar, Thomas Gleixner, Rohit Seth

On Thu, 3 Feb 2011 21:50:04 +0100 (CET)
Jesper Juhl <jj@chaosbits.net> wrote:

> In arch/x86/mm/hugetlbpage.c:huge_pmd_share() we call find_vma(mm, addr) 
> and then subsequently dereference the pointer returned without checking if 
> it was NULL. I can't find anything that guarantees that find_vma() will 
> never return NULL in this case, so I believe there's a genuine bug. 
> However, I'd greatly appreciate it if someone would take the time to 
> double check me.
> This patch implements what I believe to be the correct handling of a NULL 
> return from find_vma(). Please consider for inclusion.
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
>  hugetlbpage.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> index 069ce7c..0ebd3d0 100644
> --- a/arch/x86/mm/hugetlbpage.c
> +++ b/arch/x86/mm/hugetlbpage.c
> @@ -61,14 +61,19 @@ static int vma_shareable(struct vm_area_struct *vma, unsigned long addr)
>  static void huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
>  {
>  	struct vm_area_struct *vma = find_vma(mm, addr);
> -	struct address_space *mapping = vma->vm_file->f_mapping;
> -	pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
> -			vma->vm_pgoff;
> +	struct address_space *mapping;
> +	pgoff_t idx;
>  	struct prio_tree_iter iter;
>  	struct vm_area_struct *svma;
>  	unsigned long saddr;
>  	pte_t *spte = NULL;
>  
> +	if (!vma)
> +		return;
> +
> +	mapping = vma->vm_file->f_mapping;
> +	idx = ((addr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> +
>  	if (!vma_shareable(vma, addr))
>  		return;

mmap_sem is held and the caller (copy_hugetlb_page_range) knows that
the virtual address at `addr' is within a vma.  So it's a "can't happen".

The code's all undocumented and you got fooled.  Cause, effect.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Huge TLB: Potential NULL deref in arch/x86/mm/hugetlbpage.c:huge_pmd_share()
  2011-02-03 21:01 ` Andrew Morton
@ 2011-02-03 21:17   ` Jesper Juhl
  2011-02-03 21:36     ` Andrew Morton
  0 siblings, 1 reply; 5+ messages in thread
From: Jesper Juhl @ 2011-02-03 21:17 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, x86, linux-mm, Tejun Heo, H. Peter Anvin,
	Ingo Molnar, Thomas Gleixner, Rohit Seth

On Thu, 3 Feb 2011, Andrew Morton wrote:

> On Thu, 3 Feb 2011 21:50:04 +0100 (CET)
> Jesper Juhl <jj@chaosbits.net> wrote:
> 
> > In arch/x86/mm/hugetlbpage.c:huge_pmd_share() we call find_vma(mm, addr) 
> > and then subsequently dereference the pointer returned without checking if 
> > it was NULL. I can't find anything that guarantees that find_vma() will 
> > never return NULL in this case, so I believe there's a genuine bug. 
> > However, I'd greatly appreciate it if someone would take the time to 
> > double check me.
> > This patch implements what I believe to be the correct handling of a NULL 
> > return from find_vma(). Please consider for inclusion.
> > 
> > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > ---
> >  hugetlbpage.c |   11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> > index 069ce7c..0ebd3d0 100644
> > --- a/arch/x86/mm/hugetlbpage.c
> > +++ b/arch/x86/mm/hugetlbpage.c
> > @@ -61,14 +61,19 @@ static int vma_shareable(struct vm_area_struct *vma, unsigned long addr)
> >  static void huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
> >  {
> >  	struct vm_area_struct *vma = find_vma(mm, addr);
> > -	struct address_space *mapping = vma->vm_file->f_mapping;
> > -	pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
> > -			vma->vm_pgoff;
> > +	struct address_space *mapping;
> > +	pgoff_t idx;
> >  	struct prio_tree_iter iter;
> >  	struct vm_area_struct *svma;
> >  	unsigned long saddr;
> >  	pte_t *spte = NULL;
> >  
> > +	if (!vma)
> > +		return;
> > +
> > +	mapping = vma->vm_file->f_mapping;
> > +	idx = ((addr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> > +
> >  	if (!vma_shareable(vma, addr))
> >  		return;
> 
> mmap_sem is held and the caller (copy_hugetlb_page_range) knows that
> the virtual address at `addr' is within a vma.  So it's a "can't happen".
> 
> The code's all undocumented and you got fooled.  Cause, effect.

Thank you very much for taking the time to check and explain.

How about we then merge the following patch instead, so noone else gets 
fooled in the future?


Document that NULL deref will never happen post find_vma() in 
huge_pmd_share().

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 hugetlbpage.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
index 069ce7c..7dd2d5f 100644
--- a/arch/x86/mm/hugetlbpage.c
+++ b/arch/x86/mm/hugetlbpage.c
@@ -61,6 +61,11 @@ static int vma_shareable(struct vm_area_struct *vma, unsigned long addr)
 static void huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
 {
 	struct vm_area_struct *vma = find_vma(mm, addr);
+	/*
+	 * There is no potential NULL deref here. mmap_sem is held and
+	 * caller knows that the virtual address at `addr' is within a
+	 * vma, so find_vma() will never return NULL here.
+	 */
 	struct address_space *mapping = vma->vm_file->f_mapping;
 	pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
 			vma->vm_pgoff;


-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Huge TLB: Potential NULL deref in arch/x86/mm/hugetlbpage.c:huge_pmd_share()
  2011-02-03 21:17   ` Jesper Juhl
@ 2011-02-03 21:36     ` Andrew Morton
  2011-02-03 21:43       ` Jesper Juhl
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2011-02-03 21:36 UTC (permalink / raw)
  To: Jesper Juhl
  Cc: linux-kernel, x86, linux-mm, Tejun Heo, H. Peter Anvin,
	Ingo Molnar, Thomas Gleixner, Rohit Seth

On Thu, 3 Feb 2011 22:17:53 +0100 (CET)
Jesper Juhl <jj@chaosbits.net> wrote:

> Document that NULL deref will never happen post find_vma() in 
> huge_pmd_share().
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
>  hugetlbpage.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> index 069ce7c..7dd2d5f 100644
> --- a/arch/x86/mm/hugetlbpage.c
> +++ b/arch/x86/mm/hugetlbpage.c
> @@ -61,6 +61,11 @@ static int vma_shareable(struct vm_area_struct *vma, unsigned long addr)
>  static void huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
>  {
>  	struct vm_area_struct *vma = find_vma(mm, addr);
> +	/*
> +	 * There is no potential NULL deref here. mmap_sem is held and
> +	 * caller knows that the virtual address at `addr' is within a
> +	 * vma, so find_vma() will never return NULL here.
> +	 */
>  	struct address_space *mapping = vma->vm_file->f_mapping;
>  	pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
>  			vma->vm_pgoff;

Not really.

That mmap_sem is held and that `addr' refers to a known-to-be-present
VMA are part of huge_pmd_share()'s interface.  They are preconditions
which must be satisfied before the function can be used.

So they should be documented as such, in the function's documentation. 
In fact they're the *most important* thing to document about the
function because they are subtle and unobvious from the implementation
and from the function signature and name.

>From an understandability/maintainability POV the code is crap.  It's yet
another example of kernel developers' liking for pointing machine guns
at each other's feet.

Really, some poor schmuck needs to go in and reverse-engineer all the
secret interface requirements and document them.  But we shouldn't let
a chance go by - a nice kerneldoc description for huge_pmd_share()
would be appreciated.  One which documents both the explicit and the
implicit calling conventions.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Huge TLB: Potential NULL deref in arch/x86/mm/hugetlbpage.c:huge_pmd_share()
  2011-02-03 21:36     ` Andrew Morton
@ 2011-02-03 21:43       ` Jesper Juhl
  0 siblings, 0 replies; 5+ messages in thread
From: Jesper Juhl @ 2011-02-03 21:43 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, x86, linux-mm, Tejun Heo, H. Peter Anvin,
	Ingo Molnar, Thomas Gleixner, Rohit Seth

On Thu, 3 Feb 2011, Andrew Morton wrote:

> On Thu, 3 Feb 2011 22:17:53 +0100 (CET)
> Jesper Juhl <jj@chaosbits.net> wrote:
> 
> > Document that NULL deref will never happen post find_vma() in 
> > huge_pmd_share().
> > 
> > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > ---
> >  hugetlbpage.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> > index 069ce7c..7dd2d5f 100644
> > --- a/arch/x86/mm/hugetlbpage.c
> > +++ b/arch/x86/mm/hugetlbpage.c
> > @@ -61,6 +61,11 @@ static int vma_shareable(struct vm_area_struct *vma, unsigned long addr)
> >  static void huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
> >  {
> >  	struct vm_area_struct *vma = find_vma(mm, addr);
> > +	/*
> > +	 * There is no potential NULL deref here. mmap_sem is held and
> > +	 * caller knows that the virtual address at `addr' is within a
> > +	 * vma, so find_vma() will never return NULL here.
> > +	 */
> >  	struct address_space *mapping = vma->vm_file->f_mapping;
> >  	pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
> >  			vma->vm_pgoff;
> 
> Not really.
> 
> That mmap_sem is held and that `addr' refers to a known-to-be-present
> VMA are part of huge_pmd_share()'s interface.  They are preconditions
> which must be satisfied before the function can be used.
> 
> So they should be documented as such, in the function's documentation. 
> In fact they're the *most important* thing to document about the
> function because they are subtle and unobvious from the implementation
> and from the function signature and name.
> 
> >From an understandability/maintainability POV the code is crap.  It's yet
> another example of kernel developers' liking for pointing machine guns
> at each other's feet.
> 
> Really, some poor schmuck needs to go in and reverse-engineer all the
> secret interface requirements and document them.  But we shouldn't let
> a chance go by - a nice kerneldoc description for huge_pmd_share()
> would be appreciated.  One which documents both the explicit and the
> implicit calling conventions.
> 

Ok. I'm not sure that I'm the right person to actually do this since my 
knowledge of this code is quite limited, but I'll give it a shot tomorrow 
evening (off to get some sleep soon, so now is not the time).
I'll get back to you on this tomorrow or during the weekend (and if 
someone else beats me to it - just fine ;).

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2011-02-03 21:44 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-03 20:50 [PATCH] Huge TLB: Potential NULL deref in arch/x86/mm/hugetlbpage.c:huge_pmd_share() Jesper Juhl
2011-02-03 21:01 ` Andrew Morton
2011-02-03 21:17   ` Jesper Juhl
2011-02-03 21:36     ` Andrew Morton
2011-02-03 21:43       ` Jesper Juhl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).