PATCH - bug in vfree

PATCH - bug in vfree

[Neil Booth]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

Linus,

I posted this bug on the kernel mailing list last year, but it never got
fixed, probably as I didn't include a patch. I attach a patch this time
against kernel 2.2.1. The bug is rare, but can lead to kernel virtual
memory corruption.

Quick description:- vfree forgets to subtract the extra cushion page
from the size of each virtual memory area stored in vmlist when it calls
vmfree_area_pages. This means that only the  vmalloc-requested size is
allocated by vmalloc_area_pages, but the requested size PLUS the cushion
page is freed by vmfree_area_pages.

More deeply:- Close inspection of get_vm_area reveals that
(intentionally?) it does NOT insist there be a cushion page behind a VMA
that is placed in front of a previously-allocated VMA, it ONLY
guarantees that a cushion page lies in front of newly-allocated VMAs.
Thus two VMAs could be immediately adjacent without a cushion page, and
coupled with the vfree bug means that vfree-ing the first VMA also frees
the first page of the second VMA, with dire consequences.

I have described this as clearly as I can, I hope it makes sense. Alan,
this same bug also exists in 2.0.36.

Neil.

[-- Attachment #2: vfree-patch --]
[-- Type: text/plain, Size: 384 bytes --]

--- linux/mm/vmalloc.c~	Sun Jan 24 19:21:06 1999
+++ linux/mm/vmalloc.c	Sat Feb 20 20:17:11 1999
@@ -187,7 +187,7 @@
 	for (p = &vmlist ; (tmp = *p) ; p = &tmp->next) {
 		if (tmp->addr == addr) {
 			*p = tmp->next;
-			vmfree_area_pages(VMALLOC_VMADDR(tmp->addr), tmp->size);
+			vmfree_area_pages(VMALLOC_VMADDR(tmp->addr), tmp->size - PAGE_SIZE);
 			kfree(tmp);
 			return;
 		}

Re: PATCH - bug in vfree

[Neil Booth]

Neil Booth wrote:

> More deeply:- Close inspection of get_vm_area reveals that
> (intentionally?) it does NOT insist there be a cushion page behind a VMA
> that is placed in front of a previously-allocated VMA, it ONLY
> guarantees that a cushion page lies in front of newly-allocated VMAs.

Sorry, this is not correct (mistook < for <=). The bug report is
correct, though.

Neil.
--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address'
in the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Re: PATCH - bug in vfree

[Kanoj Sarcar]

On Feb 20,  8:46pm, Neil Booth wrote:
> Subject: PATCH - bug in vfree
>

>
> Quick description:- vfree forgets to subtract the extra cushion page
> from the size of each virtual memory area stored in vmlist when it calls
> vmfree_area_pages. This means that only the  vmalloc-requested size is
> allocated by vmalloc_area_pages, but the requested size PLUS the cushion
> page is freed by vmfree_area_pages.
>
> More deeply:- Close inspection of get_vm_area reveals that
> (intentionally?) it does NOT insist there be a cushion page behind a VMA
> that is placed in front of a previously-allocated VMA, it ONLY
> guarantees that a cushion page lies in front of newly-allocated VMAs.
> Thus two VMAs could be immediately adjacent without a cushion page, and
> coupled with the vfree bug means that vfree-ing the first VMA also frees
> the first page of the second VMA, with dire consequences.
>
> I have described this as clearly as I can, I hope it makes sense. Alan,
> this same bug also exists in 2.0.36.
>
> Neil.
>
> [ text/plain ] :
>
> --- linux/mm/vmalloc.c~	Sun Jan 24 19:21:06 1999
> +++ linux/mm/vmalloc.c	Sat Feb 20 20:17:11 1999
> @@ -187,7 +187,7 @@
>  	for (p = &vmlist ; (tmp = *p) ; p = &tmp->next) {
>  		if (tmp->addr == addr) {
>  			*p = tmp->next;
> -			vmfree_area_pages(VMALLOC_VMADDR(tmp->addr),
tmp->size);
> +			vmfree_area_pages(VMALLOC_VMADDR(tmp->addr), tmp->size
- PAGE_SIZE);
>  			kfree(tmp);
>  			return;
>  		}
>-- End of excerpt from Neil Booth


On Feb 20,  9:14pm, Neil Booth wrote:
> Subject: Re: PATCH - bug in vfree
> Neil Booth wrote:
>
> > More deeply:- Close inspection of get_vm_area reveals that
> > (intentionally?) it does NOT insist there be a cushion page behind a VMA
> > that is placed in front of a previously-allocated VMA, it ONLY
> > guarantees that a cushion page lies in front of newly-allocated VMAs.
>
> Sorry, this is not correct (mistook < for <=). The bug report is
> correct, though.
>
> Neil.


Given that we agree that there is always one page between vm_structs,
the extra page freeing (in vfree) is probably inconsequential, given
that vmfree_area_pages/free_area_pmd/free_area_pte basically ignores
null ptes. But yes, I agree it would be nice to get the "bug" fixed
in vfree().

Kanoj
--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address'
in the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Re: PATCH - bug in vfree

[Andrea Arcangeli]

On Sat, 20 Feb 1999, Neil Booth wrote:

>I posted this bug on the kernel mailing list last year, but it never got
>fixed, probably as I didn't include a patch. I attach a patch this time

I included it one year ago in my tree and infact if you grab my
arca-patches you'll find it again ;).

>against kernel 2.2.1. The bug is rare, but can lead to kernel virtual
>memory corruption.

Hmm, when I checked it one year ago I didn't seen a way the bug could
corrupt memory.

>More deeply:- Close inspection of get_vm_area reveals that
>(intentionally?) it does NOT insist there be a cushion page behind a VMA
>that is placed in front of a previously-allocated VMA, it ONLY

Could you explain me better? I agree that there's no good reason trying to
free the gap-faulting page, but I don't see how there couldn't be a
page-gap between two vmalloced areas.

Andrea Arcangeli

--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address'
in the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/

Re: PATCH - bug in vfree

[Neil Booth]

Andrea Arcangeli wrote:

> Hmm, when I checked it one year ago I didn't seen a way the bug could
> corrupt memory.

Yes, you missed my retraction of that bit below.

Neil.

Neil Booth wrote:
> 
> Neil Booth wrote:
> 
> > More deeply:- Close inspection of get_vm_area reveals that
> > (intentionally?) it does NOT insist there be a cushion page behind a VMA
> > that is placed in front of a previously-allocated VMA, it ONLY
> > guarantees that a cushion page lies in front of newly-allocated VMAs.
> 
> Sorry, this is not correct (mistook < for <=). The bug report is
> correct, though.
> 
> Neil.
--
To unsubscribe, send a message with 'unsubscribe linux-mm my@address'
in the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://humbolt.geo.uu.nl/Linux-MM/