2007-08-11 13:21:56

by Wu Fengguang

[permalink] [raw]
Subject: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

Andrew,

I'm not sure if this patch is the right fix for the bug. But it do
stops the oops message. The bug also happens in 2.6.23-rc1-mm2/2.6.23-rc2-mm2.
I'm running debian/sid. The .config is attached.


Unable to handle kernel NULL pointer dereference at 0000000000000138 RIP
[<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
PGD 7cd84067 PUD 7d9c3067 PMD 0
Oops: 0000 [2] SMP
CPU 1
Modules linked in: eeprom lm85 hwmon_vid i2c_core tun fuse kvm snd_hda_intel snd_pcm_oss snd_mixer_oss snd_pcm snd_timer sg snd soundcore sr_mod cdrom snd_page_alloc pcspkr evdev button raid0 usbhid sd_mod uhci_hcd ehci_hcd ata_piix ahci ohci1394 ieee1394 thermal processor fan
Pid: 4305, comm: khelper Tainted: G D 2.6.23-rc2 #9
RIP: 0010:[<ffffffff81092f66>] [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
RSP: 0000:ffff81007a3d1d30 EFLAGS: 00010206
RAX: 0000000000000000 RBX: 0000000000000001 RCX: 000000000003e142
RDX: 00000000000004f4 RSI: 0000000000000001 RDI: 0000000000000001
RBP: ffff81007a3d1d40 R08: ffff81007a3d1d78 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
R13: 0000000000000000 R14: ffff81007d0c0188 R15: ffff81007d577b18
FS: 0000000000000000(0000) GS:ffff81007efa11d0(0000) knlGS:0000000000000000
CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000000138 CR3: 000000003799b000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process khelper (pid: 4305, threadinfo ffff81007a3d0000, task ffff81007a30acc0)
Stack: 0000000000000001 ffff81007d577b18 ffff81007a3d1d60 ffffffff811297ff
0000000000000001 ffff810037c84bb8 ffff81007a3d1da0 ffffffff81092532
ffff81007d0c0188 0000000000000000 ffff81007d577b20 0000000000000000
Call Trace:
[<ffffffff811297ff>] cap_vm_enough_memory+0x2f/0x40
[<ffffffff81092532>] insert_vm_struct+0xa2/0xb0
[<ffffffff810b020e>] bprm_mm_init+0x11e/0x1b0
[<ffffffff810b1b71>] do_execve+0x81/0x220
[<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
[<ffffffff8100abc6>] sys_execve+0x46/0xb0
[<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
[<ffffffff8100cf64>] kernel_execve+0x64/0xd0
[<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
[<ffffffff8104c384>] ____call_usermodehelper+0x174/0x190
[<ffffffff8100cef8>] child_rip+0xa/0x12
[<ffffffff810356f8>] schedule_tail+0x78/0x100
[<ffffffff8100c60c>] restore_args+0x0/0x30
[<ffffffff8104c210>] ____call_usermodehelper+0x0/0x190
[<ffffffff8100ceee>] child_rip+0x0/0x12


Code: 48 8b 80 38 01 00 00 48 c1 e8 05 48 29 c1 48 39 ca 0f 8c 89
RIP [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
RSP <ffff81007a3d1d30>
CR2: 0000000000000138

Signed-off-by: Fengguang Wu <[email protected]>
---
mm/mmap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.23-rc2-mm2.orig/mm/mmap.c
+++ linux-2.6.23-rc2-mm2/mm/mmap.c
@@ -166,7 +166,8 @@ int __vm_enough_memory(long pages, int c

/* Don't let a single process grow too big:
leave 3% of the size of this process for other processes */
- allowed -= current->mm->total_vm / 32;
+ if (current->mm)
+ allowed -= current->mm->total_vm / 32;

/*
* cast `allowed' as a signed long because vm_committed_space


Attachments:
(No filename) (3.18 kB)
.config (51.43 kB)
Download all attachments

2007-08-11 14:17:36

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

[Fengguang Wu - Sat, Aug 11, 2007 at 09:21:31PM +0800]
| Andrew,
|
| I'm not sure if this patch is the right fix for the bug. But it do
| stops the oops message. The bug also happens in 2.6.23-rc1-mm2/2.6.23-rc2-mm2.
| I'm running debian/sid. The .config is attached.
|
|

[...snip...]

Even if you're right you have to make the same patch for
mm/nommu.c but I've an anticipation the problem is growing
up from another point (and I'm really hoping that I'm wrong ;)

Cyrill

2007-08-11 14:30:29

by Balbir Singh

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On 8/11/07, Fengguang Wu <[email protected]> wrote:
> Andrew,
>
> I'm not sure if this patch is the right fix for the bug. But it do
> stops the oops message. The bug also happens in 2.6.23-rc1-mm2/2.6.23-rc2-mm2.
> I'm running debian/sid. The .config is attached.
>
>
> Unable to handle kernel NULL pointer dereference at 0000000000000138 RIP
> [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
> PGD 7cd84067 PUD 7d9c3067 PMD 0
> Oops: 0000 [2] SMP
> CPU 1
> Modules linked in: eeprom lm85 hwmon_vid i2c_core tun fuse kvm snd_hda_intel snd_pcm_oss snd_mixer_oss snd_pcm snd_timer sg snd soundcore sr_mod cdrom snd_page_alloc pcspkr evdev button raid0 usbhid sd_mod uhci_hcd ehci_hcd ata_piix ahci ohci1394 ieee1394 thermal processor fan
> Pid: 4305, comm: khelper Tainted: G D 2.6.23-rc2 #9
> RIP: 0010:[<ffffffff81092f66>] [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
> RSP: 0000:ffff81007a3d1d30 EFLAGS: 00010206
> RAX: 0000000000000000 RBX: 0000000000000001 RCX: 000000000003e142
> RDX: 00000000000004f4 RSI: 0000000000000001 RDI: 0000000000000001
> RBP: ffff81007a3d1d40 R08: ffff81007a3d1d78 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
> R13: 0000000000000000 R14: ffff81007d0c0188 R15: ffff81007d577b18
> FS: 0000000000000000(0000) GS:ffff81007efa11d0(0000) knlGS:0000000000000000
> CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
> CR2: 0000000000000138 CR3: 000000003799b000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process khelper (pid: 4305, threadinfo ffff81007a3d0000, task ffff81007a30acc0)
> Stack: 0000000000000001 ffff81007d577b18 ffff81007a3d1d60 ffffffff811297ff
> 0000000000000001 ffff810037c84bb8 ffff81007a3d1da0 ffffffff81092532
> ffff81007d0c0188 0000000000000000 ffff81007d577b20 0000000000000000
> Call Trace:
> [<ffffffff811297ff>] cap_vm_enough_memory+0x2f/0x40
> [<ffffffff81092532>] insert_vm_struct+0xa2/0xb0
> [<ffffffff810b020e>] bprm_mm_init+0x11e/0x1b0
> [<ffffffff810b1b71>] do_execve+0x81/0x220
> [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
> [<ffffffff8100abc6>] sys_execve+0x46/0xb0
> [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
> [<ffffffff8100cf64>] kernel_execve+0x64/0xd0
> [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
> [<ffffffff8104c384>] ____call_usermodehelper+0x174/0x190
> [<ffffffff8100cef8>] child_rip+0xa/0x12
> [<ffffffff810356f8>] schedule_tail+0x78/0x100
> [<ffffffff8100c60c>] restore_args+0x0/0x30
> [<ffffffff8104c210>] ____call_usermodehelper+0x0/0x190
> [<ffffffff8100ceee>] child_rip+0x0/0x12
>
>
> Code: 48 8b 80 38 01 00 00 48 c1 e8 05 48 29 c1 48 39 ca 0f 8c 89
> RIP [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
> RSP <ffff81007a3d1d30>
> CR2: 0000000000000138
>
> Signed-off-by: Fengguang Wu <[email protected]>
> ---
> mm/mmap.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> --- linux-2.6.23-rc2-mm2.orig/mm/mmap.c
> +++ linux-2.6.23-rc2-mm2/mm/mmap.c
> @@ -166,7 +166,8 @@ int __vm_enough_memory(long pages, int c
>
> /* Don't let a single process grow too big:
> leave 3% of the size of this process for other processes */
> - allowed -= current->mm->total_vm / 32;
> + if (current->mm)
> + allowed -= current->mm->total_vm / 32;
>
> /*
> * cast `allowed' as a signed long because vm_committed_space
>
>

Shouldn't we just not stop vm accounting for kernel threads?

Warm Regards,
Balbir Singh

2007-08-11 17:00:52

by Andrew Morton

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On Sat, 11 Aug 2007 20:00:12 +0530 "Balbir Singh" <[email protected]> wrote:

> On 8/11/07, Fengguang Wu <[email protected]> wrote:

hm, both people who replied to this removed Fengguang from cc. Disagreement
between headers and MUAs, perhaps?

> > Andrew,
> >
> > I'm not sure if this patch is the right fix for the bug. But it do
> > stops the oops message. The bug also happens in 2.6.23-rc1-mm2/2.6.23-rc2-mm2.
> > I'm running debian/sid. The .config is attached.
> >
> >
> > Unable to handle kernel NULL pointer dereference at 0000000000000138 RIP
> > [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
> > PGD 7cd84067 PUD 7d9c3067 PMD 0
> > Oops: 0000 [2] SMP
> > CPU 1
> > Modules linked in: eeprom lm85 hwmon_vid i2c_core tun fuse kvm snd_hda_intel snd_pcm_oss snd_mixer_oss snd_pcm snd_timer sg snd soundcore sr_mod cdrom snd_page_alloc pcspkr evdev button raid0 usbhid sd_mod uhci_hcd ehci_hcd ata_piix ahci ohci1394 ieee1394 thermal processor fan
> > Pid: 4305, comm: khelper Tainted: G D 2.6.23-rc2 #9
> > RIP: 0010:[<ffffffff81092f66>] [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
> > RSP: 0000:ffff81007a3d1d30 EFLAGS: 00010206
> > RAX: 0000000000000000 RBX: 0000000000000001 RCX: 000000000003e142
> > RDX: 00000000000004f4 RSI: 0000000000000001 RDI: 0000000000000001
> > RBP: ffff81007a3d1d40 R08: ffff81007a3d1d78 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
> > R13: 0000000000000000 R14: ffff81007d0c0188 R15: ffff81007d577b18
> > FS: 0000000000000000(0000) GS:ffff81007efa11d0(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
> > CR2: 0000000000000138 CR3: 000000003799b000 CR4: 00000000000006e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > Process khelper (pid: 4305, threadinfo ffff81007a3d0000, task ffff81007a30acc0)
> > Stack: 0000000000000001 ffff81007d577b18 ffff81007a3d1d60 ffffffff811297ff
> > 0000000000000001 ffff810037c84bb8 ffff81007a3d1da0 ffffffff81092532
> > ffff81007d0c0188 0000000000000000 ffff81007d577b20 0000000000000000
> > Call Trace:
> > [<ffffffff811297ff>] cap_vm_enough_memory+0x2f/0x40
> > [<ffffffff81092532>] insert_vm_struct+0xa2/0xb0
> > [<ffffffff810b020e>] bprm_mm_init+0x11e/0x1b0
> > [<ffffffff810b1b71>] do_execve+0x81/0x220
> > [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
> > [<ffffffff8100abc6>] sys_execve+0x46/0xb0
> > [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
> > [<ffffffff8100cf64>] kernel_execve+0x64/0xd0
> > [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
> > [<ffffffff8104c384>] ____call_usermodehelper+0x174/0x190
> > [<ffffffff8100cef8>] child_rip+0xa/0x12
> > [<ffffffff810356f8>] schedule_tail+0x78/0x100
> > [<ffffffff8100c60c>] restore_args+0x0/0x30
> > [<ffffffff8104c210>] ____call_usermodehelper+0x0/0x190
> > [<ffffffff8100ceee>] child_rip+0x0/0x12
> >
> >
> > Code: 48 8b 80 38 01 00 00 48 c1 e8 05 48 29 c1 48 39 ca 0f 8c 89
> > RIP [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
> > RSP <ffff81007a3d1d30>
> > CR2: 0000000000000138
> >
> > Signed-off-by: Fengguang Wu <[email protected]>
> > ---
> > mm/mmap.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > --- linux-2.6.23-rc2-mm2.orig/mm/mmap.c
> > +++ linux-2.6.23-rc2-mm2/mm/mmap.c
> > @@ -166,7 +166,8 @@ int __vm_enough_memory(long pages, int c
> >
> > /* Don't let a single process grow too big:
> > leave 3% of the size of this process for other processes */
> > - allowed -= current->mm->total_vm / 32;
> > + if (current->mm)
> > + allowed -= current->mm->total_vm / 32;
> >
> > /*
> > * cast `allowed' as a signed long because vm_committed_space
> >
> >
>
> Shouldn't we just not stop vm accounting for kernel threads?
>

Could be. It'd help heaps if we knew which patch in -mm caused
this, but from a quick peek it seems to me that mainline should be
vulnerable as well.

2007-08-11 18:02:19

by Balbir Singh

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

Andrew Morton wrote:
> On Sat, 11 Aug 2007 20:00:12 +0530 "Balbir Singh" <[email protected]> wrote:
>
>> On 8/11/07, Fengguang Wu <[email protected]> wrote:
>
> hm, both people who replied to this removed Fengguang from cc. Disagreement
> between headers and MUAs, perhaps?
>

Oops... Not sure how that happened. I'll check my sent mail.

>>> Andrew,
>>>
>>> I'm not sure if this patch is the right fix for the bug. But it do
>>> stops the oops message. The bug also happens in 2.6.23-rc1-mm2/2.6.23-rc2-mm2.
>>> I'm running debian/sid. The .config is attached.
>>>
>>>
>>> Unable to handle kernel NULL pointer dereference at 0000000000000138 RIP
>>> [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
>>> PGD 7cd84067 PUD 7d9c3067 PMD 0
>>> Oops: 0000 [2] SMP
>>> CPU 1
>>> Modules linked in: eeprom lm85 hwmon_vid i2c_core tun fuse kvm snd_hda_intel snd_pcm_oss snd_mixer_oss snd_pcm snd_timer sg snd soundcore sr_mod cdrom snd_page_alloc pcspkr evdev button raid0 usbhid sd_mod uhci_hcd ehci_hcd ata_piix ahci ohci1394 ieee1394 thermal processor fan
>>> Pid: 4305, comm: khelper Tainted: G D 2.6.23-rc2 #9
>>> RIP: 0010:[<ffffffff81092f66>] [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
>>> RSP: 0000:ffff81007a3d1d30 EFLAGS: 00010206
>>> RAX: 0000000000000000 RBX: 0000000000000001 RCX: 000000000003e142
>>> RDX: 00000000000004f4 RSI: 0000000000000001 RDI: 0000000000000001
>>> RBP: ffff81007a3d1d40 R08: ffff81007a3d1d78 R09: 0000000000000000
>>> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
>>> R13: 0000000000000000 R14: ffff81007d0c0188 R15: ffff81007d577b18
>>> FS: 0000000000000000(0000) GS:ffff81007efa11d0(0000) knlGS:0000000000000000
>>> CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
>>> CR2: 0000000000000138 CR3: 000000003799b000 CR4: 00000000000006e0
>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
>>> Process khelper (pid: 4305, threadinfo ffff81007a3d0000, task ffff81007a30acc0)
>>> Stack: 0000000000000001 ffff81007d577b18 ffff81007a3d1d60 ffffffff811297ff
>>> 0000000000000001 ffff810037c84bb8 ffff81007a3d1da0 ffffffff81092532
>>> ffff81007d0c0188 0000000000000000 ffff81007d577b20 0000000000000000
>>> Call Trace:
>>> [<ffffffff811297ff>] cap_vm_enough_memory+0x2f/0x40
>>> [<ffffffff81092532>] insert_vm_struct+0xa2/0xb0
>>> [<ffffffff810b020e>] bprm_mm_init+0x11e/0x1b0
>>> [<ffffffff810b1b71>] do_execve+0x81/0x220
>>> [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
>>> [<ffffffff8100abc6>] sys_execve+0x46/0xb0
>>> [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
>>> [<ffffffff8100cf64>] kernel_execve+0x64/0xd0
>>> [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
>>> [<ffffffff8104c384>] ____call_usermodehelper+0x174/0x190
>>> [<ffffffff8100cef8>] child_rip+0xa/0x12
>>> [<ffffffff810356f8>] schedule_tail+0x78/0x100
>>> [<ffffffff8100c60c>] restore_args+0x0/0x30
>>> [<ffffffff8104c210>] ____call_usermodehelper+0x0/0x190
>>> [<ffffffff8100ceee>] child_rip+0x0/0x12
>>>
>>>
>>> Code: 48 8b 80 38 01 00 00 48 c1 e8 05 48 29 c1 48 39 ca 0f 8c 89
>>> RIP [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
>>> RSP <ffff81007a3d1d30>
>>> CR2: 0000000000000138
>>>
>>> Signed-off-by: Fengguang Wu <[email protected]>
>>> ---
>>> mm/mmap.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> --- linux-2.6.23-rc2-mm2.orig/mm/mmap.c
>>> +++ linux-2.6.23-rc2-mm2/mm/mmap.c
>>> @@ -166,7 +166,8 @@ int __vm_enough_memory(long pages, int c
>>>
>>> /* Don't let a single process grow too big:
>>> leave 3% of the size of this process for other processes */
>>> - allowed -= current->mm->total_vm / 32;
>>> + if (current->mm)
>>> + allowed -= current->mm->total_vm / 32;
>>>
>>> /*
>>> * cast `allowed' as a signed long because vm_committed_space
>>>
>>>
>> Shouldn't we just not stop vm accounting for kernel threads?
>>
>
> Could be. It'd help heaps if we knew which patch in -mm caused
> this, but from a quick peek it seems to me that mainline should be
> vulnerable as well.

Thats a valid point. It would be interesting to see what the overcommit
setting was, when the panic occurred.

--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL

2007-08-11 18:13:35

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

[Balbir Singh - Sat, Aug 11, 2007 at 11:31:09PM +0530]
| Andrew Morton wrote:
| > On Sat, 11 Aug 2007 20:00:12 +0530 "Balbir Singh" <[email protected]> wrote:
| >
| >> On 8/11/07, Fengguang Wu <[email protected]> wrote:
| >
| > hm, both people who replied to this removed Fengguang from cc. Disagreement
| > between headers and MUAs, perhaps?
| >
|
| Oops... Not sure how that happened. I'll check my sent mail.
|
| >>> Andrew,
| >>>
| >>> I'm not sure if this patch is the right fix for the bug. But it do
| >>> stops the oops message. The bug also happens in 2.6.23-rc1-mm2/2.6.23-rc2-mm2.
| >>> I'm running debian/sid. The .config is attached.
| >>>
| >>>
| >>> Unable to handle kernel NULL pointer dereference at 0000000000000138 RIP
| >>> [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
| >>> PGD 7cd84067 PUD 7d9c3067 PMD 0
| >>> Oops: 0000 [2] SMP
| >>> CPU 1
| >>> Modules linked in: eeprom lm85 hwmon_vid i2c_core tun fuse kvm snd_hda_intel snd_pcm_oss snd_mixer_oss snd_pcm snd_timer sg snd soundcore sr_mod cdrom snd_page_alloc pcspkr evdev button raid0 usbhid sd_mod uhci_hcd ehci_hcd ata_piix ahci ohci1394 ieee1394 thermal processor fan
| >>> Pid: 4305, comm: khelper Tainted: G D 2.6.23-rc2 #9
| >>> RIP: 0010:[<ffffffff81092f66>] [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
| >>> RSP: 0000:ffff81007a3d1d30 EFLAGS: 00010206
| >>> RAX: 0000000000000000 RBX: 0000000000000001 RCX: 000000000003e142
| >>> RDX: 00000000000004f4 RSI: 0000000000000001 RDI: 0000000000000001
| >>> RBP: ffff81007a3d1d40 R08: ffff81007a3d1d78 R09: 0000000000000000
| >>> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
| >>> R13: 0000000000000000 R14: ffff81007d0c0188 R15: ffff81007d577b18
| >>> FS: 0000000000000000(0000) GS:ffff81007efa11d0(0000) knlGS:0000000000000000
| >>> CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
| >>> CR2: 0000000000000138 CR3: 000000003799b000 CR4: 00000000000006e0
| >>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
| >>> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
| >>> Process khelper (pid: 4305, threadinfo ffff81007a3d0000, task ffff81007a30acc0)
| >>> Stack: 0000000000000001 ffff81007d577b18 ffff81007a3d1d60 ffffffff811297ff
| >>> 0000000000000001 ffff810037c84bb8 ffff81007a3d1da0 ffffffff81092532
| >>> ffff81007d0c0188 0000000000000000 ffff81007d577b20 0000000000000000
| >>> Call Trace:
| >>> [<ffffffff811297ff>] cap_vm_enough_memory+0x2f/0x40
| >>> [<ffffffff81092532>] insert_vm_struct+0xa2/0xb0
| >>> [<ffffffff810b020e>] bprm_mm_init+0x11e/0x1b0
| >>> [<ffffffff810b1b71>] do_execve+0x81/0x220
| >>> [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
| >>> [<ffffffff8100abc6>] sys_execve+0x46/0xb0
| >>> [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
| >>> [<ffffffff8100cf64>] kernel_execve+0x64/0xd0
| >>> [<ffffffff8104bf90>] __call_usermodehelper+0x0/0x90
| >>> [<ffffffff8104c384>] ____call_usermodehelper+0x174/0x190
| >>> [<ffffffff8100cef8>] child_rip+0xa/0x12
| >>> [<ffffffff810356f8>] schedule_tail+0x78/0x100
| >>> [<ffffffff8100c60c>] restore_args+0x0/0x30
| >>> [<ffffffff8104c210>] ____call_usermodehelper+0x0/0x190
| >>> [<ffffffff8100ceee>] child_rip+0x0/0x12
| >>>
| >>>
| >>> Code: 48 8b 80 38 01 00 00 48 c1 e8 05 48 29 c1 48 39 ca 0f 8c 89
| >>> RIP [<ffffffff81092f66>] __vm_enough_memory+0x76/0x120
| >>> RSP <ffff81007a3d1d30>
| >>> CR2: 0000000000000138
| >>>
| >>> Signed-off-by: Fengguang Wu <[email protected]>
| >>> ---
| >>> mm/mmap.c | 3 ++-
| >>> 1 file changed, 2 insertions(+), 1 deletion(-)
| >>>
| >>> --- linux-2.6.23-rc2-mm2.orig/mm/mmap.c
| >>> +++ linux-2.6.23-rc2-mm2/mm/mmap.c
| >>> @@ -166,7 +166,8 @@ int __vm_enough_memory(long pages, int c
| >>>
| >>> /* Don't let a single process grow too big:
| >>> leave 3% of the size of this process for other processes */
| >>> - allowed -= current->mm->total_vm / 32;
| >>> + if (current->mm)
| >>> + allowed -= current->mm->total_vm / 32;
| >>>
| >>> /*
| >>> * cast `allowed' as a signed long because vm_committed_space
| >>>
| >>>
| >> Shouldn't we just not stop vm accounting for kernel threads?
| >>
| >
| > Could be. It'd help heaps if we knew which patch in -mm caused
| > this, but from a quick peek it seems to me that mainline should be
| > vulnerable as well.
|
| Thats a valid point. It would be interesting to see what the overcommit
| setting was, when the panic occurred.

May I suggest - could it be commit b6a2fea39318e43fee84fa7b0b90d68bed92d2ba
Not sure but take a look please

Cyrill

2007-08-12 05:29:32

by Wu Fengguang

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On Sat, Aug 11, 2007 at 06:17:14PM +0400, Cyrill Gorcunov wrote:
> [Fengguang Wu - Sat, Aug 11, 2007 at 09:21:31PM +0800]
> | Andrew,
> |
> | I'm not sure if this patch is the right fix for the bug. But it do
> | stops the oops message. The bug also happens in 2.6.23-rc1-mm2/2.6.23-rc2-mm2.
> | I'm running debian/sid. The .config is attached.
> |
> |
>
> [...snip...]
>
> Even if you're right you have to make the same patch for
> mm/nommu.c but I've an anticipation the problem is growing
> up from another point (and I'm really hoping that I'm wrong ;)

Thank you, the patch is updated to:
===

Fix possible NULL pointer deference on __vm_enough_memory().

Cc: Cyrill Gorcunov <[email protected]>
Signed-off-by: Fengguang Wu <[email protected]>
---
mm/mmap.c | 3 ++-
mm/nommu.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)

--- linux-2.6.23-rc2-mm2.orig/mm/mmap.c
+++ linux-2.6.23-rc2-mm2/mm/mmap.c
@@ -166,7 +166,8 @@ int __vm_enough_memory(long pages, int c

/* Don't let a single process grow too big:
leave 3% of the size of this process for other processes */
- allowed -= current->mm->total_vm / 32;
+ if (current->mm)
+ allowed -= current->mm->total_vm / 32;

/*
* cast `allowed' as a signed long because vm_committed_space
--- linux-2.6.23-rc2-mm2.orig/mm/nommu.c
+++ linux-2.6.23-rc2-mm2/mm/nommu.c
@@ -1342,7 +1342,8 @@ int __vm_enough_memory(long pages, int c

/* Don't let a single process grow too big:
leave 3% of the size of this process for other processes */
- allowed -= current->mm->total_vm / 32;
+ if (current->mm)
+ allowed -= current->mm->total_vm / 32;

/*
* cast `allowed' as a signed long because vm_committed_space

2007-08-12 05:45:48

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

[Fengguang Wu - Sun, Aug 12, 2007 at 01:29:15PM +0800]
| On Sat, Aug 11, 2007 at 06:17:14PM +0400, Cyrill Gorcunov wrote:
| > [Fengguang Wu - Sat, Aug 11, 2007 at 09:21:31PM +0800]
| > | Andrew,
| > |
| > | I'm not sure if this patch is the right fix for the bug. But it do
| > | stops the oops message. The bug also happens in 2.6.23-rc1-mm2/2.6.23-rc2-mm2.
| > | I'm running debian/sid. The .config is attached.
| > |
| > |
| >
| > [...snip...]
| >
| > Even if you're right you have to make the same patch for
| > mm/nommu.c but I've an anticipation the problem is growing
| > up from another point (and I'm really hoping that I'm wrong ;)
|
| Thank you, the patch is updated to:
| ===
|
| Fix possible NULL pointer deference on __vm_enough_memory().
|
| Cc: Cyrill Gorcunov <[email protected]>
| Signed-off-by: Fengguang Wu <[email protected]>
| ---
| mm/mmap.c | 3 ++-
| mm/nommu.c | 3 ++-
| 2 files changed, 4 insertions(+), 2 deletions(-)
|
| --- linux-2.6.23-rc2-mm2.orig/mm/mmap.c
| +++ linux-2.6.23-rc2-mm2/mm/mmap.c
| @@ -166,7 +166,8 @@ int __vm_enough_memory(long pages, int c
|
| /* Don't let a single process grow too big:
| leave 3% of the size of this process for other processes */
| - allowed -= current->mm->total_vm / 32;
| + if (current->mm)
| + allowed -= current->mm->total_vm / 32;
|
| /*
| * cast `allowed' as a signed long because vm_committed_space
| --- linux-2.6.23-rc2-mm2.orig/mm/nommu.c
| +++ linux-2.6.23-rc2-mm2/mm/nommu.c
| @@ -1342,7 +1342,8 @@ int __vm_enough_memory(long pages, int c
|
| /* Don't let a single process grow too big:
| leave 3% of the size of this process for other processes */
| - allowed -= current->mm->total_vm / 32;
| + if (current->mm)
| + allowed -= current->mm->total_vm / 32;
|
| /*
| * cast `allowed' as a signed long because vm_committed_space
|

ok, lets wait for some mature developer comments

Cyrill

2007-08-12 05:46:23

by Wu Fengguang

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On Sat, Aug 11, 2007 at 10:00:15AM -0700, Andrew Morton wrote:
> On Sat, 11 Aug 2007 20:00:12 +0530 "Balbir Singh" <[email protected]> wrote:
>
> > On 8/11/07, Fengguang Wu <[email protected]> wrote:
>
> hm, both people who replied to this removed Fengguang from cc. Disagreement
> between headers and MUAs, perhaps?

Hehe, my email setup is somehow complicated. I'd better not to intermix
[email protected] and [email protected] in one single mail :)

2007-08-12 05:48:44

by Wu Fengguang

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On Sat, Aug 11, 2007 at 11:31:09PM +0530, Balbir Singh wrote:
> Andrew Morton wrote:
> > On Sat, 11 Aug 2007 20:00:12 +0530 "Balbir Singh" <[email protected]> wrote:
> >>>
> >> Shouldn't we just not stop vm accounting for kernel threads?
> >>
> >
> > Could be. It'd help heaps if we knew which patch in -mm caused
> > this, but from a quick peek it seems to me that mainline should be
> > vulnerable as well.
>
> Thats a valid point. It would be interesting to see what the overcommit
> setting was, when the panic occurred.

FYI, I do have nondefault overcommit settings:

vm.overcommit_memory = 2
vm.lowmem_reserve_ratio = 1 1

2007-08-12 08:58:24

by Wu Fengguang

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On Sun, Aug 12, 2007 at 01:48:31PM +0800, WU Fengguang wrote:
> On Sat, Aug 11, 2007 at 11:31:09PM +0530, Balbir Singh wrote:
> > Andrew Morton wrote:
> > > On Sat, 11 Aug 2007 20:00:12 +0530 "Balbir Singh" <[email protected]> wrote:
> > >>>
> > >> Shouldn't we just not stop vm accounting for kernel threads?
> > >>
> > >
> > > Could be. It'd help heaps if we knew which patch in -mm caused
> > > this, but from a quick peek it seems to me that mainline should be
> > > vulnerable as well.
> >
> > Thats a valid point. It would be interesting to see what the overcommit
> > setting was, when the panic occurred.
>
> FYI, I do have nondefault overcommit settings:
>
> vm.overcommit_memory = 2
> vm.lowmem_reserve_ratio = 1 1

Yes, the bug disappears when changing to default overcommit_memory!

2007-08-12 09:26:01

by Balbir Singh

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

WU Fengguang wrote:
> On Sun, Aug 12, 2007 at 01:48:31PM +0800, WU Fengguang wrote:
>> On Sat, Aug 11, 2007 at 11:31:09PM +0530, Balbir Singh wrote:
>>> Andrew Morton wrote:
>>>> On Sat, 11 Aug 2007 20:00:12 +0530 "Balbir Singh" <[email protected]> wrote:
>>>>> Shouldn't we just not stop vm accounting for kernel threads?
>>>>>
>>>> Could be. It'd help heaps if we knew which patch in -mm caused
>>>> this, but from a quick peek it seems to me that mainline should be
>>>> vulnerable as well.
>>> Thats a valid point. It would be interesting to see what the overcommit
>>> setting was, when the panic occurred.
>> FYI, I do have nondefault overcommit settings:
>>
>> vm.overcommit_memory = 2
>> vm.lowmem_reserve_ratio = 1 1
>
> Yes, the bug disappears when changing to default overcommit_memory!
>

Great! So the problem might have existed for some time, but we never
saw it due to default over commit values? Were you using these values
for over commit even before?

--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL

2007-08-12 12:23:44

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

[Balbir Singh - Sun, Aug 12, 2007 at 02:55:32PM +0530]
| WU Fengguang wrote:
| > On Sun, Aug 12, 2007 at 01:48:31PM +0800, WU Fengguang wrote:
| >> On Sat, Aug 11, 2007 at 11:31:09PM +0530, Balbir Singh wrote:
| >>> Andrew Morton wrote:
| >>>> On Sat, 11 Aug 2007 20:00:12 +0530 "Balbir Singh" <[email protected]> wrote:
| >>>>> Shouldn't we just not stop vm accounting for kernel threads?
| >>>>>
| >>>> Could be. It'd help heaps if we knew which patch in -mm caused
| >>>> this, but from a quick peek it seems to me that mainline should be
| >>>> vulnerable as well.
| >>> Thats a valid point. It would be interesting to see what the overcommit
| >>> setting was, when the panic occurred.
| >> FYI, I do have nondefault overcommit settings:
| >>
| >> vm.overcommit_memory = 2
| >> vm.lowmem_reserve_ratio = 1 1
| >
| > Yes, the bug disappears when changing to default overcommit_memory!
| >
|
| Great! So the problem might have existed for some time, but we never
| saw it due to default over commit values? Were you using these values
| for over commit even before?
|
| --
| Warm Regards,
| Balbir Singh
| Linux Technology Center
| IBM, ISTL

So the problem is that __vm_enough_memory is just _not_ capable
to be called from a thread with OVERCOMMIT_NEVER and Fengguang's
patch does fix it. The scenario Fengguang show us is growing
from keventd that starts a new user task. So Fengguang's patch
seems right to me ;)

Cyrill

2007-08-12 12:28:05

by Wu Fengguang

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

Hi Balbir,

[add CC to LKML]

On Sun, Aug 12, 2007 at 05:27:52PM +0530, Balbir Singh wrote:
> For some reason my mailer keeps removing you from the cc.

Or maybe it's my SMTP server's problem. Email systems are complex.

> >>> Thats a valid point. It would be interesting to see what the overcommit
> >>> setting was, when the panic occurred.
> >> FYI, I do have nondefault overcommit settings:
> >>
> >> vm.overcommit_memory = 2
> >> vm.lowmem_reserve_ratio = 1 1
> >
> > Yes, the bug disappears when changing to default overcommit_memory!
> >
>
> Great! So the problem might have existed for some time, but we never
> saw it due to default over commit values? Were you using these values
> for over commit even before?

No I changed it several weeks ago to stop my desktop from freezing.
So yes, the bug may have been there for a while.

2007-08-12 13:12:20

by Alan

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

> > Great! So the problem might have existed for some time, but we never
> > saw it due to default over commit values? Were you using these values
> > for over commit even before?
>
> No I changed it several weeks ago to stop my desktop from freezing.
> So yes, the bug may have been there for a while.

The bug is the new exec with lots of arguments code. It tries to insert a
vm struct without having a valid current->mm. That isn't permitted and
never had been (which is also why it broke the sparc mmu code etc).

You'll need to change the kernel security interface a little to make this
fly - I think the following should do it.

- make __vm_enough_memory take a struct mm pointer and use it
- make security_ops pass the extra current->mm
- add a vm_enough_memory_mm security op
- use security_vm_enough_memory_mm(mm, ...) in __insert_vm_struct

I'll knock up a quick patch and see what is needed (someone else can do
the selinux changes)

2007-08-12 14:09:34

by Wu Fengguang

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On Sun, Aug 12, 2007 at 02:19:05PM +0100, Alan Cox wrote:
> > > Great! So the problem might have existed for some time, but we never
> > > saw it due to default over commit values? Were you using these values
> > > for over commit even before?
> >
> > No I changed it several weeks ago to stop my desktop from freezing.
> > So yes, the bug may have been there for a while.
>
> The bug is the new exec with lots of arguments code. It tries to insert a
> vm struct without having a valid current->mm. That isn't permitted and
> never had been (which is also why it broke the sparc mmu code etc).
>
> You'll need to change the kernel security interface a little to make this
> fly - I think the following should do it.
>
> - make __vm_enough_memory take a struct mm pointer and use it
> - make security_ops pass the extra current->mm
> - add a vm_enough_memory_mm security op
> - use security_vm_enough_memory_mm(mm, ...) in __insert_vm_struct
>
> I'll knock up a quick patch and see what is needed (someone else can do
> the selinux changes)

Thank you!

Count me for one - but CC SELinux maintainers first :)

2007-08-12 15:17:46

by Alan

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

Try this (it compiles but isnt tested). Its a weekend here, the sun is
shining, the beach is a short walk, and I have more interesting things to
do right now 8)


diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/include/linux/mm.h linux-2.6.23rc1-mm1/include/linux/mm.h
--- linux.vanilla-2.6.23rc1-mm1/include/linux/mm.h 2007-07-26 15:02:58.000000000 +0100
+++ linux-2.6.23rc1-mm1/include/linux/mm.h 2007-08-12 13:54:24.614647536 +0100
@@ -1079,7 +1079,7 @@
}

/* mmap.c */
-extern int __vm_enough_memory(long pages, int cap_sys_admin);
+extern int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin);
extern void vma_adjust(struct vm_area_struct *vma, unsigned long start,
unsigned long end, pgoff_t pgoff, struct vm_area_struct *insert);
extern struct vm_area_struct *vma_merge(struct mm_struct *,
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/include/linux/security.h linux-2.6.23rc1-mm1/include/linux/security.h
--- linux.vanilla-2.6.23rc1-mm1/include/linux/security.h 2007-07-26 15:02:58.000000000 +0100
+++ linux-2.6.23rc1-mm1/include/linux/security.h 2007-08-12 14:13:10.383504656 +0100
@@ -58,7 +58,7 @@
extern int cap_task_setioprio (struct task_struct *p, int ioprio);
extern int cap_task_setnice (struct task_struct *p, int nice);
extern int cap_syslog (int type);
-extern int cap_vm_enough_memory (long pages);
+extern int cap_vm_enough_memory (struct mm_struct *mm, long pages);

struct msghdr;
struct sk_buff;
@@ -1129,6 +1129,7 @@
* Return 0 if permission is granted.
* @vm_enough_memory:
* Check permissions for allocating a new virtual mapping.
+ * @mm contains the mm struct it is being added to.
* @pages contains the number of pages.
* Return 0 if permission is granted.
*
@@ -1173,7 +1174,7 @@
int (*quota_on) (struct dentry * dentry);
int (*syslog) (int type);
int (*settime) (struct timespec *ts, struct timezone *tz);
- int (*vm_enough_memory) (long pages);
+ int (*vm_enough_memory) (struct mm_struct *mm, long pages);

int (*bprm_alloc_security) (struct linux_binprm * bprm);
void (*bprm_free_security) (struct linux_binprm * bprm);
@@ -1439,6 +1440,7 @@
int security_syslog(int type);
int security_settime(struct timespec *ts, struct timezone *tz);
int security_vm_enough_memory(long pages);
+int security_vm_enough_memory_mm(struct mm_struct *mm, long pages);
int security_bprm_alloc(struct linux_binprm *bprm);
void security_bprm_free(struct linux_binprm *bprm);
void security_bprm_apply_creds(struct linux_binprm *bprm, int unsafe);
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/mm/mmap.c linux-2.6.23rc1-mm1/mm/mmap.c
--- linux.vanilla-2.6.23rc1-mm1/mm/mmap.c 2007-07-26 15:02:58.000000000 +0100
+++ linux-2.6.23rc1-mm1/mm/mmap.c 2007-08-12 13:53:22.000000000 +0100
@@ -93,7 +93,7 @@
* Note this is a helper function intended to be used by LSMs which
* wish to use this logic.
*/
-int __vm_enough_memory(long pages, int cap_sys_admin)
+int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
{
unsigned long free, allowed;

@@ -166,7 +166,7 @@

/* Don't let a single process grow too big:
leave 3% of the size of this process for other processes */
- allowed -= current->mm->total_vm / 32;
+ allowed -= mm->total_vm / 32;

/*
* cast `allowed' as a signed long because vm_committed_space
@@ -2058,7 +2058,7 @@
if (__vma && __vma->vm_start < vma->vm_end)
return -ENOMEM;
if ((vma->vm_flags & VM_ACCOUNT) &&
- security_vm_enough_memory(vma_pages(vma)))
+ security_vm_enough_memory_mm(mm, vma_pages(vma)))
return -ENOMEM;
vma_link(mm, vma, prev, rb_link, rb_parent);
return 0;
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/mm/nommu.c linux-2.6.23rc1-mm1/mm/nommu.c
--- linux.vanilla-2.6.23rc1-mm1/mm/nommu.c 2007-07-26 15:02:08.000000000 +0100
+++ linux-2.6.23rc1-mm1/mm/nommu.c 2007-08-12 13:53:57.000000000 +0100
@@ -1270,7 +1270,7 @@
* Note this is a helper function intended to be used by LSMs which
* wish to use this logic.
*/
-int __vm_enough_memory(long pages, int cap_sys_admin)
+int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
{
unsigned long free, allowed;

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/commoncap.c linux-2.6.23rc1-mm1/security/commoncap.c
--- linux.vanilla-2.6.23rc1-mm1/security/commoncap.c 2007-07-26 15:02:59.000000000 +0100
+++ linux-2.6.23rc1-mm1/security/commoncap.c 2007-08-12 14:13:29.000000000 +0100
@@ -489,13 +489,13 @@
return 0;
}

-int cap_vm_enough_memory(long pages)
+int cap_vm_enough_memory(struct mm_struct *mm, long pages)
{
int cap_sys_admin = 0;

if (cap_capable(current, CAP_SYS_ADMIN) == 0)
cap_sys_admin = 1;
- return __vm_enough_memory(pages, cap_sys_admin);
+ return __vm_enough_memory(mm, pages, cap_sys_admin);
}

EXPORT_SYMBOL(cap_capable);
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/dummy.c linux-2.6.23rc1-mm1/security/dummy.c
--- linux.vanilla-2.6.23rc1-mm1/security/dummy.c 2007-07-26 15:02:59.000000000 +0100
+++ linux-2.6.23rc1-mm1/security/dummy.c 2007-08-12 14:10:49.000000000 +0100
@@ -107,13 +107,13 @@
return 0;
}

-static int dummy_vm_enough_memory(long pages)
+static int dummy_vm_enough_memory(struct mm_struct *mm, long pages)
{
int cap_sys_admin = 0;

if (dummy_capable(current, CAP_SYS_ADMIN) == 0)
cap_sys_admin = 1;
- return __vm_enough_memory(pages, cap_sys_admin);
+ return __vm_enough_memory(mm, pages, cap_sys_admin);
}

static int dummy_bprm_alloc_security (struct linux_binprm *bprm)
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/security.c linux-2.6.23rc1-mm1/security/security.c
--- linux.vanilla-2.6.23rc1-mm1/security/security.c 2007-07-26 15:02:59.000000000 +0100
+++ linux-2.6.23rc1-mm1/security/security.c 2007-08-12 13:47:53.000000000 +0100
@@ -237,10 +237,14 @@
return security_ops->settime(ts, tz);
}

-
int security_vm_enough_memory(long pages)
{
- return security_ops->vm_enough_memory(pages);
+ return security_ops->vm_enough_memory(current->mm, pages);
+}
+
+int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
+{
+ return security_ops->vm_enough_memory(mm, pages);
}

int security_bprm_alloc(struct linux_binprm *bprm)
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/selinux/hooks.c linux-2.6.23rc1-mm1/security/selinux/hooks.c
--- linux.vanilla-2.6.23rc1-mm1/security/selinux/hooks.c 2007-07-26 15:02:59.000000000 +0100
+++ linux-2.6.23rc1-mm1/security/selinux/hooks.c 2007-08-12 14:11:21.000000000 +0100
@@ -1584,7 +1584,7 @@
* Do not audit the selinux permission check, as this is applied to all
* processes that allocate mappings.
*/
-static int selinux_vm_enough_memory(long pages)
+static int selinux_vm_enough_memory(struct mm_struct *mm, long pages)
{
int rc, cap_sys_admin = 0;
struct task_security_struct *tsec = current->security;
@@ -1600,7 +1600,7 @@
if (rc == 0)
cap_sys_admin = 1;

- return __vm_enough_memory(pages, cap_sys_admin);
+ return __vm_enough_memory(mm, pages, cap_sys_admin);
}

/* binprm security operations */

2007-08-12 16:22:07

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

[Alan Cox - Sun, Aug 12, 2007 at 04:17:44PM +0100]
| Try this (it compiles but isnt tested). Its a weekend here, the sun is
| shining, the beach is a short walk, and I have more interesting things to
| do right now 8)
|
|
| diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/include/linux/mm.h linux-2.6.23rc1-mm1/include/linux/mm.h
| --- linux.vanilla-2.6.23rc1-mm1/include/linux/mm.h 2007-07-26 15:02:58.000000000 +0100
| +++ linux-2.6.23rc1-mm1/include/linux/mm.h 2007-08-12 13:54:24.614647536 +0100
| @@ -1079,7 +1079,7 @@
| }
|
| /* mmap.c */
| -extern int __vm_enough_memory(long pages, int cap_sys_admin);
| +extern int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin);
| extern void vma_adjust(struct vm_area_struct *vma, unsigned long start,
| unsigned long end, pgoff_t pgoff, struct vm_area_struct *insert);
| extern struct vm_area_struct *vma_merge(struct mm_struct *,
| diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/include/linux/security.h linux-2.6.23rc1-mm1/include/linux/security.h
| --- linux.vanilla-2.6.23rc1-mm1/include/linux/security.h 2007-07-26 15:02:58.000000000 +0100
| +++ linux-2.6.23rc1-mm1/include/linux/security.h 2007-08-12 14:13:10.383504656 +0100
| @@ -58,7 +58,7 @@
| extern int cap_task_setioprio (struct task_struct *p, int ioprio);
| extern int cap_task_setnice (struct task_struct *p, int nice);
| extern int cap_syslog (int type);
| -extern int cap_vm_enough_memory (long pages);
| +extern int cap_vm_enough_memory (struct mm_struct *mm, long pages);
|
| struct msghdr;
| struct sk_buff;
| @@ -1129,6 +1129,7 @@
| * Return 0 if permission is granted.
| * @vm_enough_memory:
| * Check permissions for allocating a new virtual mapping.
| + * @mm contains the mm struct it is being added to.
| * @pages contains the number of pages.
| * Return 0 if permission is granted.
| *
| @@ -1173,7 +1174,7 @@
| int (*quota_on) (struct dentry * dentry);
| int (*syslog) (int type);
| int (*settime) (struct timespec *ts, struct timezone *tz);
| - int (*vm_enough_memory) (long pages);
| + int (*vm_enough_memory) (struct mm_struct *mm, long pages);
|
| int (*bprm_alloc_security) (struct linux_binprm * bprm);
| void (*bprm_free_security) (struct linux_binprm * bprm);
| @@ -1439,6 +1440,7 @@
| int security_syslog(int type);
| int security_settime(struct timespec *ts, struct timezone *tz);
| int security_vm_enough_memory(long pages);
| +int security_vm_enough_memory_mm(struct mm_struct *mm, long pages);
| int security_bprm_alloc(struct linux_binprm *bprm);
| void security_bprm_free(struct linux_binprm *bprm);
| void security_bprm_apply_creds(struct linux_binprm *bprm, int unsafe);
| diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/mm/mmap.c linux-2.6.23rc1-mm1/mm/mmap.c
| --- linux.vanilla-2.6.23rc1-mm1/mm/mmap.c 2007-07-26 15:02:58.000000000 +0100
| +++ linux-2.6.23rc1-mm1/mm/mmap.c 2007-08-12 13:53:22.000000000 +0100
| @@ -93,7 +93,7 @@
| * Note this is a helper function intended to be used by LSMs which
| * wish to use this logic.
| */
| -int __vm_enough_memory(long pages, int cap_sys_admin)
| +int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
| {
| unsigned long free, allowed;
|
| @@ -166,7 +166,7 @@
|
| /* Don't let a single process grow too big:
| leave 3% of the size of this process for other processes */
| - allowed -= current->mm->total_vm / 32;
| + allowed -= mm->total_vm / 32;

So mm->total_vm is 0 for __bprm_mm_init case. Is that ok? Or I miss
something?

|
| /*
| * cast `allowed' as a signed long because vm_committed_space
| @@ -2058,7 +2058,7 @@
| if (__vma && __vma->vm_start < vma->vm_end)
| return -ENOMEM;
| if ((vma->vm_flags & VM_ACCOUNT) &&
| - security_vm_enough_memory(vma_pages(vma)))
| + security_vm_enough_memory_mm(mm, vma_pages(vma)))
| return -ENOMEM;
| vma_link(mm, vma, prev, rb_link, rb_parent);
| return 0;
| diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/mm/nommu.c linux-2.6.23rc1-mm1/mm/nommu.c
| --- linux.vanilla-2.6.23rc1-mm1/mm/nommu.c 2007-07-26 15:02:08.000000000 +0100
| +++ linux-2.6.23rc1-mm1/mm/nommu.c 2007-08-12 13:53:57.000000000 +0100
| @@ -1270,7 +1270,7 @@
| * Note this is a helper function intended to be used by LSMs which
| * wish to use this logic.
| */
| -int __vm_enough_memory(long pages, int cap_sys_admin)
| +int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
| {
| unsigned long free, allowed;
|
| diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/commoncap.c linux-2.6.23rc1-mm1/security/commoncap.c
| --- linux.vanilla-2.6.23rc1-mm1/security/commoncap.c 2007-07-26 15:02:59.000000000 +0100
| +++ linux-2.6.23rc1-mm1/security/commoncap.c 2007-08-12 14:13:29.000000000 +0100
| @@ -489,13 +489,13 @@
| return 0;
| }
|
| -int cap_vm_enough_memory(long pages)
| +int cap_vm_enough_memory(struct mm_struct *mm, long pages)
| {
| int cap_sys_admin = 0;
|
| if (cap_capable(current, CAP_SYS_ADMIN) == 0)
| cap_sys_admin = 1;
| - return __vm_enough_memory(pages, cap_sys_admin);
| + return __vm_enough_memory(mm, pages, cap_sys_admin);
| }
|
| EXPORT_SYMBOL(cap_capable);
| diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/dummy.c linux-2.6.23rc1-mm1/security/dummy.c
| --- linux.vanilla-2.6.23rc1-mm1/security/dummy.c 2007-07-26 15:02:59.000000000 +0100
| +++ linux-2.6.23rc1-mm1/security/dummy.c 2007-08-12 14:10:49.000000000 +0100
| @@ -107,13 +107,13 @@
| return 0;
| }
|
| -static int dummy_vm_enough_memory(long pages)
| +static int dummy_vm_enough_memory(struct mm_struct *mm, long pages)
| {
| int cap_sys_admin = 0;
|
| if (dummy_capable(current, CAP_SYS_ADMIN) == 0)
| cap_sys_admin = 1;
| - return __vm_enough_memory(pages, cap_sys_admin);
| + return __vm_enough_memory(mm, pages, cap_sys_admin);
| }
|
| static int dummy_bprm_alloc_security (struct linux_binprm *bprm)
| diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/security.c linux-2.6.23rc1-mm1/security/security.c
| --- linux.vanilla-2.6.23rc1-mm1/security/security.c 2007-07-26 15:02:59.000000000 +0100
| +++ linux-2.6.23rc1-mm1/security/security.c 2007-08-12 13:47:53.000000000 +0100
| @@ -237,10 +237,14 @@
| return security_ops->settime(ts, tz);
| }
|
| -
| int security_vm_enough_memory(long pages)
| {
| - return security_ops->vm_enough_memory(pages);
| + return security_ops->vm_enough_memory(current->mm, pages);
| +}
| +
| +int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
| +{
| + return security_ops->vm_enough_memory(mm, pages);
| }
|
| int security_bprm_alloc(struct linux_binprm *bprm)
| diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/selinux/hooks.c linux-2.6.23rc1-mm1/security/selinux/hooks.c
| --- linux.vanilla-2.6.23rc1-mm1/security/selinux/hooks.c 2007-07-26 15:02:59.000000000 +0100
| +++ linux-2.6.23rc1-mm1/security/selinux/hooks.c 2007-08-12 14:11:21.000000000 +0100
| @@ -1584,7 +1584,7 @@
| * Do not audit the selinux permission check, as this is applied to all
| * processes that allocate mappings.
| */
| -static int selinux_vm_enough_memory(long pages)
| +static int selinux_vm_enough_memory(struct mm_struct *mm, long pages)
| {
| int rc, cap_sys_admin = 0;
| struct task_security_struct *tsec = current->security;
| @@ -1600,7 +1600,7 @@
| if (rc == 0)
| cap_sys_admin = 1;
|
| - return __vm_enough_memory(pages, cap_sys_admin);
| + return __vm_enough_memory(mm, pages, cap_sys_admin);
| }
|
| /* binprm security operations */

Cyrill

2007-08-13 00:18:12

by Rene Herman

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On 08/12/2007 05:17 PM, Alan Cox wrote:

> Try this (it compiles but isnt tested). Its a weekend here, the sun is
> shining, the beach is a short walk, and I have more interesting things to
> do right now 8)

Oh come on, you have a beard. You can't go to the beach.

Rene.

2007-08-13 00:23:57

by Wu Fengguang

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On Sun, Aug 12, 2007 at 08:21:43PM +0400, Cyrill Gorcunov wrote:
> [Alan Cox - Sun, Aug 12, 2007 at 04:17:44PM +0100]
> | Try this (it compiles but isnt tested). Its a weekend here, the sun is
> | shining, the beach is a short walk, and I have more interesting things to
> | do right now 8)
> |
> |
[...]
> | -int __vm_enough_memory(long pages, int cap_sys_admin)
> | +int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
> | {
> | unsigned long free, allowed;
> |
> | @@ -166,7 +166,7 @@
> |
> | /* Don't let a single process grow too big:
> | leave 3% of the size of this process for other processes */
> | - allowed -= current->mm->total_vm / 32;
> | + allowed -= mm->total_vm / 32;
>
> So mm->total_vm is 0 for __bprm_mm_init case. Is that ok? Or I miss
> something?

Yeah, Alan adds mm to the interfaces and leaves us the question of
"what mm to pass in when current->mm == NULL?" ;)

2007-08-13 12:51:07

by Wu Fengguang

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On Sun, Aug 12, 2007 at 04:17:44PM +0100, Alan Cox wrote:
> Try this (it compiles but isnt tested). Its a weekend here, the sun is
> shining, the beach is a short walk, and I have more interesting things to
> do right now 8)

Have a nice day~~~ It works!

> --- linux.vanilla-2.6.23rc1-mm1/mm/mmap.c 2007-07-26 15:02:58.000000000 +0100
> +++ linux-2.6.23rc1-mm1/mm/mmap.c 2007-08-12 13:53:22.000000000 +0100
> @@ -2058,7 +2058,7 @@
> if (__vma && __vma->vm_start < vma->vm_end)
> return -ENOMEM;
> if ((vma->vm_flags & VM_ACCOUNT) &&
> - security_vm_enough_memory(vma_pages(vma)))
> + security_vm_enough_memory_mm(mm, vma_pages(vma)))
> return -ENOMEM;
> vma_link(mm, vma, prev, rb_link, rb_parent);
> return 0;

Sorry, I overlooked this chunk in int insert_vm_struct(mm, vma), hehe.

2007-08-13 13:38:24

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

[WU Fengguang - Mon, Aug 13, 2007 at 08:23:42AM +0800]
| On Sun, Aug 12, 2007 at 08:21:43PM +0400, Cyrill Gorcunov wrote:
| > [Alan Cox - Sun, Aug 12, 2007 at 04:17:44PM +0100]
| > | Try this (it compiles but isnt tested). Its a weekend here, the sun is
| > | shining, the beach is a short walk, and I have more interesting things to
| > | do right now 8)
| > |
| > |
| [...]
| > | -int __vm_enough_memory(long pages, int cap_sys_admin)
| > | +int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
| > | {
| > | unsigned long free, allowed;
| > |
| > | @@ -166,7 +166,7 @@
| > |
| > | /* Don't let a single process grow too big:
| > | leave 3% of the size of this process for other processes */
| > | - allowed -= current->mm->total_vm / 32;
| > | + allowed -= mm->total_vm / 32;
| >
| > So mm->total_vm is 0 for __bprm_mm_init case. Is that ok? Or I miss
| > something?
|
| Yeah, Alan adds mm to the interfaces and leaves us the question of
| "what mm to pass in when current->mm == NULL?" ;)
|

Well, as I see, it seems the Alan's patch is correct. We pass
newly created mm to security_vm_enough_memory_mm() and get no errors
here even for overcommit = 2. But my question was that mm->total_vm
= 0 for this case and that is probably valid too I think. What about
the thing you pointed about? Well I think security_vm_enough_memory
should never be called from kernel thread (we have secrurity_vm_enough_memory_mm
for this). But I will check it more closely. Dont get me wrong - I'm not
VMM expert and may do errors ;)

Cyrill

2007-08-13 14:15:34

by Alan

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

> Well, as I see, it seems the Alan's patch is correct. We pass
> newly created mm to security_vm_enough_memory_mm() and get no errors
> here even for overcommit = 2. But my question was that mm->total_vm
> = 0 for this case and that is probably valid too I think. What about
> the thing you pointed about? Well I think security_vm_enough_memory
> should never be called from kernel thread (we have secrurity_vm_enough_memory_mm
> for this). But I will check it more closely. Dont get me wrong - I'm not
> VMM expert and may do errors ;)

A vma has to inserted into an mm struct so we are fine in terms of kernel
threads. init_bprm showed up a new case where we add vma's to an mm that
isn't current->mm. The rest of the vm subsystem supports this and there
are cases for the future (eg the usermode linux mm switching patch) where
it might matter that we do it right.

Alan

2007-08-13 14:45:54

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

[Alan Cox - Mon, Aug 13, 2007 at 12:22:24PM +0100]
| > Well, as I see, it seems the Alan's patch is correct. We pass
| > newly created mm to security_vm_enough_memory_mm() and get no errors
| > here even for overcommit = 2. But my question was that mm->total_vm
| > = 0 for this case and that is probably valid too I think. What about
| > the thing you pointed about? Well I think security_vm_enough_memory
| > should never be called from kernel thread (we have secrurity_vm_enough_memory_mm
| > for this). But I will check it more closely. Dont get me wrong - I'm not
| > VMM expert and may do errors ;)
|
| A vma has to inserted into an mm struct so we are fine in terms of kernel
| threads. init_bprm showed up a new case where we add vma's to an mm that
| isn't current->mm. The rest of the vm subsystem supports this and there
| are cases for the future (eg the usermode linux mm switching patch) where
| it might matter that we do it right.
|
| Alan
|

ok, thanks for explanation, Alan.

Cyrill

2007-08-13 15:12:52

by Alan

[permalink] [raw]
Subject: [PATCH] fix NULL pointer dereference in __vm_enough_memory()

On Mon, 13 Aug 2007 15:38:53 +0800
WU Fengguang <[email protected]> wrote:

> On Sun, Aug 12, 2007 at 04:17:44PM +0100, Alan Cox wrote:
> > Try this (it compiles but isnt tested). Its a weekend here, the sun is
> > shining, the beach is a short walk, and I have more interesting things to
> > do right now 8)
>
> Have a nice day~~~ It works!

Ok please apply the patch then Andrew

---

The new exec code inserts an accounted vma into an mm struct which is not
current->mm. The existing memory check code has a hard coded assumption
that this does not happen as does the security code.

As the correct mm is known we pass the mm to the security method and the
helper function. A new security test is added for the case where we need
to pass the mm and the existing one is modified to pass current->mm to
avoid the need to change large amounts of code.

Signed-off-by: Alan Cox <[email protected]>


diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/include/linux/mm.h linux-2.6.23rc1-mm1/include/linux/mm.h
--- linux.vanilla-2.6.23rc1-mm1/include/linux/mm.h 2007-07-26 15:02:58.000000000 +0100
+++ linux-2.6.23rc1-mm1/include/linux/mm.h 2007-08-12 13:54:24.614647536 +0100
@@ -1079,7 +1079,7 @@
}

/* mmap.c */
-extern int __vm_enough_memory(long pages, int cap_sys_admin);
+extern int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin);
extern void vma_adjust(struct vm_area_struct *vma, unsigned long start,
unsigned long end, pgoff_t pgoff, struct vm_area_struct *insert);
extern struct vm_area_struct *vma_merge(struct mm_struct *,
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/include/linux/security.h linux-2.6.23rc1-mm1/include/linux/security.h
--- linux.vanilla-2.6.23rc1-mm1/include/linux/security.h 2007-07-26 15:02:58.000000000 +0100
+++ linux-2.6.23rc1-mm1/include/linux/security.h 2007-08-12 14:13:10.383504656 +0100
@@ -58,7 +58,7 @@
extern int cap_task_setioprio (struct task_struct *p, int ioprio);
extern int cap_task_setnice (struct task_struct *p, int nice);
extern int cap_syslog (int type);
-extern int cap_vm_enough_memory (long pages);
+extern int cap_vm_enough_memory (struct mm_struct *mm, long pages);

struct msghdr;
struct sk_buff;
@@ -1129,6 +1129,7 @@
* Return 0 if permission is granted.
* @vm_enough_memory:
* Check permissions for allocating a new virtual mapping.
+ * @mm contains the mm struct it is being added to.
* @pages contains the number of pages.
* Return 0 if permission is granted.
*
@@ -1173,7 +1174,7 @@
int (*quota_on) (struct dentry * dentry);
int (*syslog) (int type);
int (*settime) (struct timespec *ts, struct timezone *tz);
- int (*vm_enough_memory) (long pages);
+ int (*vm_enough_memory) (struct mm_struct *mm, long pages);

int (*bprm_alloc_security) (struct linux_binprm * bprm);
void (*bprm_free_security) (struct linux_binprm * bprm);
@@ -1439,6 +1440,7 @@
int security_syslog(int type);
int security_settime(struct timespec *ts, struct timezone *tz);
int security_vm_enough_memory(long pages);
+int security_vm_enough_memory_mm(struct mm_struct *mm, long pages);
int security_bprm_alloc(struct linux_binprm *bprm);
void security_bprm_free(struct linux_binprm *bprm);
void security_bprm_apply_creds(struct linux_binprm *bprm, int unsafe);
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/mm/mmap.c linux-2.6.23rc1-mm1/mm/mmap.c
--- linux.vanilla-2.6.23rc1-mm1/mm/mmap.c 2007-07-26 15:02:58.000000000 +0100
+++ linux-2.6.23rc1-mm1/mm/mmap.c 2007-08-12 13:53:22.000000000 +0100
@@ -93,7 +93,7 @@
* Note this is a helper function intended to be used by LSMs which
* wish to use this logic.
*/
-int __vm_enough_memory(long pages, int cap_sys_admin)
+int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
{
unsigned long free, allowed;

@@ -166,7 +166,7 @@

/* Don't let a single process grow too big:
leave 3% of the size of this process for other processes */
- allowed -= current->mm->total_vm / 32;
+ allowed -= mm->total_vm / 32;

/*
* cast `allowed' as a signed long because vm_committed_space
@@ -2058,7 +2058,7 @@
if (__vma && __vma->vm_start < vma->vm_end)
return -ENOMEM;
if ((vma->vm_flags & VM_ACCOUNT) &&
- security_vm_enough_memory(vma_pages(vma)))
+ security_vm_enough_memory_mm(mm, vma_pages(vma)))
return -ENOMEM;
vma_link(mm, vma, prev, rb_link, rb_parent);
return 0;
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/mm/nommu.c linux-2.6.23rc1-mm1/mm/nommu.c
--- linux.vanilla-2.6.23rc1-mm1/mm/nommu.c 2007-07-26 15:02:08.000000000 +0100
+++ linux-2.6.23rc1-mm1/mm/nommu.c 2007-08-12 13:53:57.000000000 +0100
@@ -1270,7 +1270,7 @@
* Note this is a helper function intended to be used by LSMs which
* wish to use this logic.
*/
-int __vm_enough_memory(long pages, int cap_sys_admin)
+int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
{
unsigned long free, allowed;

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/commoncap.c linux-2.6.23rc1-mm1/security/commoncap.c
--- linux.vanilla-2.6.23rc1-mm1/security/commoncap.c 2007-07-26 15:02:59.000000000 +0100
+++ linux-2.6.23rc1-mm1/security/commoncap.c 2007-08-12 14:13:29.000000000 +0100
@@ -489,13 +489,13 @@
return 0;
}

-int cap_vm_enough_memory(long pages)
+int cap_vm_enough_memory(struct mm_struct *mm, long pages)
{
int cap_sys_admin = 0;

if (cap_capable(current, CAP_SYS_ADMIN) == 0)
cap_sys_admin = 1;
- return __vm_enough_memory(pages, cap_sys_admin);
+ return __vm_enough_memory(mm, pages, cap_sys_admin);
}

EXPORT_SYMBOL(cap_capable);
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/dummy.c linux-2.6.23rc1-mm1/security/dummy.c
--- linux.vanilla-2.6.23rc1-mm1/security/dummy.c 2007-07-26 15:02:59.000000000 +0100
+++ linux-2.6.23rc1-mm1/security/dummy.c 2007-08-12 14:10:49.000000000 +0100
@@ -107,13 +107,13 @@
return 0;
}

-static int dummy_vm_enough_memory(long pages)
+static int dummy_vm_enough_memory(struct mm_struct *mm, long pages)
{
int cap_sys_admin = 0;

if (dummy_capable(current, CAP_SYS_ADMIN) == 0)
cap_sys_admin = 1;
- return __vm_enough_memory(pages, cap_sys_admin);
+ return __vm_enough_memory(mm, pages, cap_sys_admin);
}

static int dummy_bprm_alloc_security (struct linux_binprm *bprm)
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/security.c linux-2.6.23rc1-mm1/security/security.c
--- linux.vanilla-2.6.23rc1-mm1/security/security.c 2007-07-26 15:02:59.000000000 +0100
+++ linux-2.6.23rc1-mm1/security/security.c 2007-08-12 13:47:53.000000000 +0100
@@ -237,10 +237,14 @@
return security_ops->settime(ts, tz);
}

-
int security_vm_enough_memory(long pages)
{
- return security_ops->vm_enough_memory(pages);
+ return security_ops->vm_enough_memory(current->mm, pages);
+}
+
+int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
+{
+ return security_ops->vm_enough_memory(mm, pages);
}

int security_bprm_alloc(struct linux_binprm *bprm)
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/security/selinux/hooks.c linux-2.6.23rc1-mm1/security/selinux/hooks.c
--- linux.vanilla-2.6.23rc1-mm1/security/selinux/hooks.c 2007-07-26 15:02:59.000000000 +0100
+++ linux-2.6.23rc1-mm1/security/selinux/hooks.c 2007-08-12 14:11:21.000000000 +0100
@@ -1584,7 +1584,7 @@
* Do not audit the selinux permission check, as this is applied to all
* processes that allocate mappings.
*/
-static int selinux_vm_enough_memory(long pages)
+static int selinux_vm_enough_memory(struct mm_struct *mm, long pages)
{
int rc, cap_sys_admin = 0;
struct task_security_struct *tsec = current->security;
@@ -1600,7 +1600,7 @@
if (rc == 0)
cap_sys_admin = 1;

- return __vm_enough_memory(pages, cap_sys_admin);
+ return __vm_enough_memory(mm, pages, cap_sys_admin);
}

/* binprm security operations */

2007-08-14 05:02:39

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] fix NULL pointer dereference in __vm_enough_memory()

On Mon, 13 Aug 2007 14:01:06 +0100 Alan Cox <[email protected]> wrote:

> On Mon, 13 Aug 2007 15:38:53 +0800
> WU Fengguang <[email protected]> wrote:
>
> > On Sun, Aug 12, 2007 at 04:17:44PM +0100, Alan Cox wrote:
> > > Try this (it compiles but isnt tested). Its a weekend here, the sun is
> > > shining, the beach is a short walk, and I have more interesting things to
> > > do right now 8)
> >
> > Have a nice day~~~ It works!
>
> Ok please apply the patch then Andrew
>
> ---
>
> The new exec code inserts an accounted vma into an mm struct which is not
> current->mm. The existing memory check code has a hard coded assumption
> that this does not happen as does the security code.
>
> As the correct mm is known we pass the mm to the security method and the
> helper function. A new security test is added for the case where we need
> to pass the mm and the existing one is modified to pass current->mm to
> avoid the need to change large amounts of code.
>
> Signed-off-by: Alan Cox <[email protected]>
>
>
> diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc1-mm1/include/linux/mm.h linux-2.6.23rc1-mm1/include/linux/mm.h
> --- linux.vanilla-2.6.23rc1-mm1/include/linux/mm.h 2007-07-26 15:02:58.000000000 +0100

erp, we have major version skew between 2.6.23-rc1-mm1 and 2.6.23-rc3 here, sorry.

Could I ask for a redone patch against mainline?

Thanks.

2007-08-14 17:38:06

by Andy Isaacson

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On Sun, Aug 12, 2007 at 08:27:46PM +0800, WU Fengguang wrote:
> On Sun, Aug 12, 2007 at 05:27:52PM +0530, Balbir Singh wrote:
> > For some reason my mailer keeps removing you from the cc.
>
> Or maybe it's my SMTP server's problem. Email systems are complex.

It's the Mail-Followup-To header you include in your emails:

> Mail-Followup-To: Balbir Singh <[email protected]>,
> Andrew Morton <[email protected]>,
> linux-kernel <[email protected]>

The mail clients are just doing what you asked. Perhaps you need to
turn off followup_to (and turn on edit_headers).

Mutt has "set honor_followup_to=ask-yes" to fix this in 1.5 (though it's
broken in "1.4.2.2i"); I don't know if the GUI clients have such a
switch.

-andy

2007-08-14 17:51:03

by Tobias Diedrich

[permalink] [raw]
Subject: Re: [PATCH] fix NULL pointer dereference in __vm_enough_memory()

Andrew Morton wrote:

> erp, we have major version skew between 2.6.23-rc1-mm1 and 2.6.23-rc3 here, sorry.
>
> Could I ask for a redone patch against mainline?

Since I'm seeing this problem on 2.6.23-rc3, I tried adapting it.
I'm running it now and the NULL pointer messages are gone.

Index: linux-2.6.23-rc3/include/linux/mm.h
===================================================================
--- linux-2.6.23-rc3.orig/include/linux/mm.h 2007-08-14 18:33:09.000000000 +0200
+++ linux-2.6.23-rc3/include/linux/mm.h 2007-08-14 19:31:59.000000000 +0200
@@ -1042,7 +1042,7 @@
}

/* mmap.c */
-extern int __vm_enough_memory(long pages, int cap_sys_admin);
+extern int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin);
extern void vma_adjust(struct vm_area_struct *vma, unsigned long start,
unsigned long end, pgoff_t pgoff, struct vm_area_struct *insert);
extern struct vm_area_struct *vma_merge(struct mm_struct *,
Index: linux-2.6.23-rc3/include/linux/security.h
===================================================================
--- linux-2.6.23-rc3.orig/include/linux/security.h 2007-08-14 18:33:10.000000000 +0200
+++ linux-2.6.23-rc3/include/linux/security.h 2007-08-14 19:38:24.000000000 +0200
@@ -54,7 +54,7 @@
extern int cap_task_post_setuid (uid_t old_ruid, uid_t old_euid, uid_t old_suid, int flags);
extern void cap_task_reparent_to_init (struct task_struct *p);
extern int cap_syslog (int type);
-extern int cap_vm_enough_memory (long pages);
+extern int cap_vm_enough_memory (struct mm_struct *mm, long pages);

struct msghdr;
struct sk_buff;
@@ -1125,6 +1125,7 @@
* Return 0 if permission is granted.
* @vm_enough_memory:
* Check permissions for allocating a new virtual mapping.
+ * @mm contains the mm struct it is being added to.
* @pages contains the number of pages.
* Return 0 if permission is granted.
*
@@ -1169,7 +1170,7 @@
int (*quota_on) (struct dentry * dentry);
int (*syslog) (int type);
int (*settime) (struct timespec *ts, struct timezone *tz);
- int (*vm_enough_memory) (long pages);
+ int (*vm_enough_memory) (struct mm_struct *mm, long pages);

int (*bprm_alloc_security) (struct linux_binprm * bprm);
void (*bprm_free_security) (struct linux_binprm * bprm);
@@ -1469,10 +1470,14 @@
return security_ops->settime(ts, tz);
}

-
static inline int security_vm_enough_memory(long pages)
{
- return security_ops->vm_enough_memory(pages);
+ return security_ops->vm_enough_memory(current->mm, pages);
+}
+
+static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
+{
+ return security_ops->vm_enough_memory(mm, pages);
}

static inline int security_bprm_alloc (struct linux_binprm *bprm)
@@ -2219,7 +2224,12 @@

static inline int security_vm_enough_memory(long pages)
{
- return cap_vm_enough_memory(pages);
+ return cap_vm_enough_memory(current->mm, pages);
+}
+
+static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
+{
+ return cap_vm_enough_memory(mm, pages);
}

static inline int security_bprm_alloc (struct linux_binprm *bprm)
Index: linux-2.6.23-rc3/mm/mmap.c
===================================================================
--- linux-2.6.23-rc3.orig/mm/mmap.c 2007-08-14 18:33:11.000000000 +0200
+++ linux-2.6.23-rc3/mm/mmap.c 2007-08-14 19:31:59.000000000 +0200
@@ -93,7 +93,7 @@
* Note this is a helper function intended to be used by LSMs which
* wish to use this logic.
*/
-int __vm_enough_memory(long pages, int cap_sys_admin)
+int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
{
unsigned long free, allowed;

@@ -166,7 +166,7 @@

/* Don't let a single process grow too big:
leave 3% of the size of this process for other processes */
- allowed -= current->mm->total_vm / 32;
+ allowed -= mm->total_vm / 32;

/*
* cast `allowed' as a signed long because vm_committed_space
@@ -2077,7 +2077,7 @@
if (__vma && __vma->vm_start < vma->vm_end)
return -ENOMEM;
if ((vma->vm_flags & VM_ACCOUNT) &&
- security_vm_enough_memory(vma_pages(vma)))
+ security_vm_enough_memory_mm(mm, vma_pages(vma)))
return -ENOMEM;
vma_link(mm, vma, prev, rb_link, rb_parent);
return 0;
Index: linux-2.6.23-rc3/mm/nommu.c
===================================================================
--- linux-2.6.23-rc3.orig/mm/nommu.c 2007-08-14 18:33:12.000000000 +0200
+++ linux-2.6.23-rc3/mm/nommu.c 2007-08-14 19:31:59.000000000 +0200
@@ -1270,7 +1270,7 @@
* Note this is a helper function intended to be used by LSMs which
* wish to use this logic.
*/
-int __vm_enough_memory(long pages, int cap_sys_admin)
+int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
{
unsigned long free, allowed;

Index: linux-2.6.23-rc3/security/commoncap.c
===================================================================
--- linux-2.6.23-rc3.orig/security/commoncap.c 2007-08-14 18:33:13.000000000 +0200
+++ linux-2.6.23-rc3/security/commoncap.c 2007-08-14 19:31:59.000000000 +0200
@@ -315,13 +315,13 @@
return 0;
}

-int cap_vm_enough_memory(long pages)
+int cap_vm_enough_memory(struct mm_struct *mm, long pages)
{
int cap_sys_admin = 0;

if (cap_capable(current, CAP_SYS_ADMIN) == 0)
cap_sys_admin = 1;
- return __vm_enough_memory(pages, cap_sys_admin);
+ return __vm_enough_memory(mm, pages, cap_sys_admin);
}

EXPORT_SYMBOL(cap_capable);
Index: linux-2.6.23-rc3/security/dummy.c
===================================================================
--- linux-2.6.23-rc3.orig/security/dummy.c 2007-08-14 18:33:13.000000000 +0200
+++ linux-2.6.23-rc3/security/dummy.c 2007-08-14 19:31:59.000000000 +0200
@@ -108,13 +108,13 @@
return 0;
}

-static int dummy_vm_enough_memory(long pages)
+static int dummy_vm_enough_memory(struct mm_struct *mm, long pages)
{
int cap_sys_admin = 0;

if (dummy_capable(current, CAP_SYS_ADMIN) == 0)
cap_sys_admin = 1;
- return __vm_enough_memory(pages, cap_sys_admin);
+ return __vm_enough_memory(mm, pages, cap_sys_admin);
}

static int dummy_bprm_alloc_security (struct linux_binprm *bprm)
Index: linux-2.6.23-rc3/security/selinux/hooks.c
===================================================================
--- linux-2.6.23-rc3.orig/security/selinux/hooks.c 2007-08-14 18:33:13.000000000 +0200
+++ linux-2.6.23-rc3/security/selinux/hooks.c 2007-08-14 19:31:59.000000000 +0200
@@ -1584,7 +1584,7 @@
* Do not audit the selinux permission check, as this is applied to all
* processes that allocate mappings.
*/
-static int selinux_vm_enough_memory(long pages)
+static int selinux_vm_enough_memory(struct mm_struct *mm, long pages)
{
int rc, cap_sys_admin = 0;
struct task_security_struct *tsec = current->security;
@@ -1600,7 +1600,7 @@
if (rc == 0)
cap_sys_admin = 1;

- return __vm_enough_memory(pages, cap_sys_admin);
+ return __vm_enough_memory(mm, pages, cap_sys_admin);
}

/* binprm security operations */

--
Tobias PGP: http://9ac7e0bc.uguu.de
$B$3$N%a!<%k$O==3d:FMxMQ$5$l$?%S%C%H$G:n$i$l$F$$$^$9!#(B

2007-08-15 08:53:30

by Wu Fengguang

[permalink] [raw]
Subject: Re: [BUGFIX] NULL pointer dereference in __vm_enough_memory()

On Tue, Aug 14, 2007 at 10:10:58AM -0700, Andy Isaacson wrote:
> On Sun, Aug 12, 2007 at 08:27:46PM +0800, WU Fengguang wrote:
> > On Sun, Aug 12, 2007 at 05:27:52PM +0530, Balbir Singh wrote:
> > > For some reason my mailer keeps removing you from the cc.
> >
> > Or maybe it's my SMTP server's problem. Email systems are complex.
>
> It's the Mail-Followup-To header you include in your emails:
>
> > Mail-Followup-To: Balbir Singh <[email protected]>,
> > Andrew Morton <[email protected]>,
> > linux-kernel <[email protected]>
>
> The mail clients are just doing what you asked. Perhaps you need to
> turn off followup_to (and turn on edit_headers).
>
> Mutt has "set honor_followup_to=ask-yes" to fix this in 1.5 (though it's
> broken in "1.4.2.2i"); I don't know if the GUI clients have such a
> switch.

Thank you for the tip, it does the trick!

Andrew: I just realized that a false muttrc was "corrected". What a fool!