FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Ubuntu Kernel Team

 
 
LinkBack Thread Tools
 
Old 06-21-2011, 03:55 PM
Stefan Bader
 
Default SRU: Fix OMG how did this ever work (32bit)

This is a multi-part message in MIME format.SRU Justification:

Impact: For i386 PGDs are stored in a linked list. For this two elements of
struct page are (mis-)used. To have a backwards pointer, the private field is
assigned a pointer to the index field of the previous struct page. The main
problem there was that list_add and list_del operations accidentally were done
twice. Which leads to accesses to (after first list operation) innocent struct
pages.

Fix: This is a bit more than needed to fix the bug itself, but it will bring our
code more into a shape that resembles upstream (factually there is only a 2.6.18
upstream but that code did not do the double list access).

Testcase: Running a 32bit domU (64bit Hardy dom0, though that should not matter)
with the xen kernel and doing a lot of process starts (like the aslr qa
regression test does) would quite soon crash because the destructor of a PTE
(which incidentally is stored in index) was suddenly overwritten.

Patches are attached directly. Those would go into the patches directory.

Note, that I attributed it to fix bug 705562 which has not yet been verified.
But the way of failure was just exactly the same.

-Stefan
 
Old 06-22-2011, 07:41 AM
Stefan Bader
 
Default SRU: Fix OMG how did this ever work (32bit)

On 21.06.2011 17:55, Stefan Bader wrote:
> SRU Justification:
>
> Impact: For i386 PGDs are stored in a linked list. For this two elements of
> struct page are (mis-)used. To have a backwards pointer, the private field is
> assigned a pointer to the index field of the previous struct page. The main
> problem there was that list_add and list_del operations accidentally were done
> twice. Which leads to accesses to (after first list operation) innocent struct
> pages.
>
> Fix: This is a bit more than needed to fix the bug itself, but it will bring our
> code more into a shape that resembles upstream (factually there is only a 2.6.18
> upstream but that code did not do the double list access).
>
> Testcase: Running a 32bit domU (64bit Hardy dom0, though that should not matter)
> with the xen kernel and doing a lot of process starts (like the aslr qa
> regression test does) would quite soon crash because the destructor of a PTE
> (which incidentally is stored in index) was suddenly overwritten.
>
> Patches are attached directly. Those would go into the patches directory.
>
> Note, that I attributed it to fix bug 705562 which has not yet been verified.
> But the way of failure was just exactly the same.
>
> -Stefan
>
After a nights sleep some additional notes. The list add operation in reality
was not a problem. That section was disabled by ifndef CONFIG_XEN and I wonder
whether I should add this case again. On the other hand this code will never be
compiled without CONFIG_XEN defined...

The list delete problem can be covered by the fact that the second delete was
only done when the quicklists number of elements get trimmed down. And that does
not happen all the times.

And at least some relaxing news that at a first glance Lucid is not affected by
this. List add and delete are only done in the constructor and destructor
functions and those seem to be only called once from alloc and free.
-Stefan

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 06-22-2011, 08:35 AM
Andy Whitcroft
 
Default SRU: Fix OMG how did this ever work (32bit)

On Tue, Jun 21, 2011 at 05:55:36PM +0200, Stefan Bader wrote:

> Fix: This is a bit more than needed to fix the bug itself, but it will bring our
> code more into a shape that resembles upstream (factually there is only a 2.6.18
> upstream but that code did not do the double list access).

The first patch looks fine, the second patch is just enormous. I am not
against making the code match upstream better as that vastly improves its
maintainability going forward. So perhaps we could split the second one
out into one "Reorder the code to match upstream order -- no functional
change" which I can then 'ignore' and one which fixes the actual bug.

Obviously even with the larger change we are going to need to test,
test, test but I believe you have this in hand and have a test case.

-apw

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 06-22-2011, 01:42 PM
Stefan Bader
 
Default SRU: Fix OMG how did this ever work (32bit)

After Andy's feedback and looking a bit closer to what the 2.6.24
version of the file looks like, it seems that it is probably better
to refrain from changing too much code.

IOW it is better to look like 2.6.24 than like the 2.6.18 xen repo
we can check against.

This does actually reduce the changes to a few things that are much
simpler to look at. So I split the changes into three indipendant
findings. The first two are more theoretical but probably best fixed
right now when I remember.

The last one is the actual issue in pgd_dtor.

-Stefan
 
Old 06-22-2011, 01:45 PM
Stefan Bader
 
Default SRU: Fix OMG how did this ever work (32bit)

On 21.06.2011 17:55, Stefan Bader wrote:
> SRU Justification:
>
> Impact: For i386 PGDs are stored in a linked list. For this two elements of
> struct page are (mis-)used. To have a backwards pointer, the private field is
> assigned a pointer to the index field of the previous struct page. The main
> problem there was that list_add and list_del operations accidentally were done
> twice. Which leads to accesses to (after first list operation) innocent struct
> pages.
>
> Fix: This is a bit more than needed to fix the bug itself, but it will bring our
> code more into a shape that resembles upstream (factually there is only a 2.6.18
> upstream but that code did not do the double list access).
>
> Testcase: Running a 32bit domU (64bit Hardy dom0, though that should not matter)
> with the xen kernel and doing a lot of process starts (like the aslr qa
> regression test does) would quite soon crash because the destructor of a PTE
> (which incidentally is stored in index) was suddenly overwritten.
>
> Patches are attached directly. Those would go into the patches directory.
>
> Note, that I attributed it to fix bug 705562 which has not yet been verified.
> But the way of failure was just exactly the same.
>
> -Stefan
>
Darn, git send-email did not work as expected here. One may guess but 2,3 and 4
are the 3 individual patches, while 1 is the one against the hardy tree.

-Stefan

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 

Thread Tools




All times are GMT. The time now is 11:06 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org