locks on unlinked files leak memory in apparmor

Bug #329489 reported by Domas Mituzas
264
Affects Status Importance Assigned to Milestone
apparmor (Ubuntu)
Invalid
Medium
Kees Cook
Hardy
Invalid
Medium
Kees Cook
Intrepid
Invalid
Medium
Kees Cook
Jaunty
Invalid
Medium
Kees Cook
linux (Ubuntu)
Fix Released
Medium
Tim Gardner
Hardy
Fix Released
Medium
Andy Whitcroft
Intrepid
Fix Released
Undecided
Andy Whitcroft
Jaunty
Fix Released
Medium
Tim Gardner

Bug Description

Apparmor is leaks memory when unlinked files are locked by confined processes.

TEST CASE

Confining the following program:

#include <stdio.h>
#include <unistd.h>

int main(void)
{
        int i;
        int fd = open("/tmp/.lockfile", O_RDWR|O_CREAT);
        unlink("/tmp/.lockfile");

        fork();
        fork();
        fork();
        fork();

        for (i = 0; i < 5000; i++) {
                struct flock lock;
                lock.l_type = F_WRLCK;
                lock.l_start = 0;
                lock.l_whence = SEEK_END;
                lock.l_len = 0;
                fcntl(fd,F_SETLKW, &lock);
                lock.l_type = F_UNLCK;
                fcntl(fd, F_SETLKW, &lock);
        }

        return 0;
}

with an apparmor policy similar to the following (place the policy in /etc/apparmor.d and then do 'sudo /etc/init.d/apparmor restart' to reload policy):

#include <tunables/global>
/PATH/TO/YOUR/COMPILED/BINARY flags=(audit) {
  #include <abstractions/base>
  #include <abstractions/mysql>
  #include <abstractions/nameservice>

  capability kill,
  capability net_bind_service,
  capability setgid,
  capability setuid,

  # Major libs
  /lib/ld-*.so mr,
  /lib/libc-*.so mr,
  /lib/libpthread-*.so mr,
  /lib/librt-*.so mr,

  /tmp/* rwk,

}

(You'll need to change /PATH/TO/YOUR/COMPILED/BINARY in the above profile to point the location of the compiled program.)

While running slabtop in another terminal, run the program.
Without the fix, slabtop should see an increase use of kernel memory, typically the kamlloc-256 slab.
With the fix in place, there shouldn't be much change in slabtop's reported output.

/var/log/messages should get a number of audit events (this confirms that confinement is applied to the binary in question).

===

SRU Justification

Justification: apparmour will leak memory leading to OOM, hangs, or crashes

Impact: certain workloads will cause memory to be leaked for each operation, specifically any name check on removed files

Fix Description: free the memory earlier in the error path

Patch: http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-jaunty.git;a=commit;h=537f85127f05d91f3a464cc3c1808fd8ea75c606

Risks: very well contained and obvious change so this should be low risk. patch is upstream (apparmour)

TEST CASE: see above

Revision history for this message
Domas Mituzas (domas-mituzas) wrote :
Revision history for this message
Domas Mituzas (domas-mituzas) wrote :

and, of course:

Linux srv47 2.6.24-19-server #1 SMP Wed Aug 20 18:43:06 UTC 2008 x86_64 GNU/Linux

hardy with all updates

Revision history for this message
Kees Cook (kees) wrote :

Thanks for the report! If AppArmor is disabled, does the leak go away? What is the specific configuration items in apache, and the common workload on the machine? I.e. what is the best way to reproduce this problem starting from a stock install of Hardy?

Changed in apparmor:
assignee: nobody → kees
status: New → Incomplete
Revision history for this message
Domas Mituzas (domas-mituzas) wrote :

testcase:
#include <fcntl.h>
#include <stdio.h>

main() {

int fd = open("/tmp/.lockfile", O_RDWR|O_CREAT);
unlink("/tmp/.lockfile");

fork();
fork();
fork();
fork();

while(1) {
 struct flock lock;
 lock.l_type=F_WRLCK;
 lock.l_start=0;
 lock.l_whence=SEEK_END;
 lock.l_len=0;
 fcntl(fd,F_SETLKW,&lock);
 lock.l_type=F_UNLCK;
 fcntl(fd,F_SETLKW,&lock);
}

}

Profile:
# Last Modified: Fri Dec 5 13:59:51 2008
#include <tunables/global>
/usr/local/sbin/domasaatest flags=(audit) {
  #include <abstractions/base>
  #include <abstractions/mysql>
  #include <abstractions/nameservice>

  capability kill,
  capability net_bind_service,
  capability setgid,
  capability setuid,

  # Major libs
  /lib/ld-*.so mr,
  /lib/libc-*.so mr,
  /lib/libpthread-*.so mr,
  /lib/librt-*.so mr,

  /tmp/* rwk,

}

Revision history for this message
Domas Mituzas (domas-mituzas) wrote :

Testcase immediately leaks memory, as well as kernel message buffer gets lots of smashed buffer crap:

http://p.defau.lt/?HxHScO_HJyFcgLVE2Fp8qw

Revision history for this message
Domas Mituzas (domas-mituzas) wrote :

One more detail, this is excerpt from /proc/slabinfo:

kmalloc-1024 806 816 1024 4 1 : tunables 0 0 0 : slabdata 204 204 0
kmalloc-512 517 528 512 8 1 : tunables 0 0 0 : slabdata 66 66 0
kmalloc-256 907300 907328 256 16 1 : tunables 0 0 0 : slabdata 56708 56708 0
kmalloc-128 566 704 128 32 1 : tunables 0 0 0 : slabdata 22 22 0
kmalloc-64 28608 28800 64 64 1 : tunables 0 0 0 : slabdata 450 450 0

the kmalloc-256 is the one that doesn't stop growing

Revision history for this message
Kees Cook (kees) wrote :

Yup, your testcase killed my hardy vm, but only when AppArmor was confining the testcase.

Changed in apparmor:
importance: Undecided → Medium
status: Incomplete → Confirmed
Revision history for this message
Kees Cook (kees) wrote :

This affects intrepid and jaunty as well.

Changed in apparmor:
status: New → Confirmed
assignee: nobody → kees
importance: Undecided → Medium
status: New → Confirmed
assignee: nobody → kees
importance: Undecided → Medium
Revision history for this message
Steve Beattie (sbeattie) wrote : Re: [Bug 329489] Re: frequent fcntl()s leak memory in apparmor

On Sat, Feb 14, 2009 at 09:16:37PM -0000, Kees Cook wrote:
> Yup, your testcase killed my hardy vm, but only when AppArmor was
> confining the testcase.

Does it do the same if the audit flag is removed from the profile?

--
Steve Beattie
<email address hidden>
http://NxNW.org/~steve/

Revision history for this message
Domas Mituzas (domas-mituzas) wrote :

Steve,

it does same in all modes:

aa-complain
aa-audit
aa-enforce

Revision history for this message
Domas Mituzas (domas-mituzas) wrote : Re: fcntl() locks on unlinked files leak memory in apparmor

OK, sorry folks for having too difficult start, I ended up with extremely simple testcase, leaking 100MB/s:

#include <fcntl.h>
#include <stdio.h>

main() {
    int fd = open("/tmp/.lockfile", O_RDWR | O_CREAT);
    unlink("/tmp/.lockfile");
    while (1) write(fd,"a",1);
}

I'm changing the issue summary yet again.

Revision history for this message
Steve Beattie (sbeattie) wrote : Re: frequent fcntl()s leak memory in apparmor

It looks like the issue is the constructed pathname dealing with the deleted file; commenting out the unlink() call in the testcase causes no memory growth, in any of the modes (you do need to remember to manually delete the lockfile if it exists before each testrun).

(FYI, you can use slabtop to see the growth. I also limited the while loop locally to do 1000 iterations, which makes the leak visible, but doesn't cause the kernel to crash.)

Revision history for this message
Domas Mituzas (domas-mituzas) wrote : Re: operations on unlinked files leak memory in apparmor

How did you make the loop run only 1000 times?!!!? :)
Thanks for 'slabtop' hint - nice tool for tourists from userland.

Now, let me put my MySQL hat on:
- MySQL ships by default with apparmor profile
- MySQL uses unlinked files quite a bit (implicit temptables, sort buffer overflows, etc)
So, this affects mysql-server package a lot too.

Revision history for this message
Domas Mituzas (domas-mituzas) wrote :

damn, after all, it may be just fcntl() - I can't reproduce the write() case anymore, and I wonder if I ever could.
teaches me that C is a compiled language!

Revision history for this message
Steve Beattie (sbeattie) wrote : Re: [Bug 329489] [NEW] frequent fcntl()s leak memory in apparmor

Upstream (jjohansen) thinks that
https://forgesvn1.novell.com/viewsvn/apparmor?view=rev&revision=1380
fixes the issue; I haven't tested it yet.

--
Steve Beattie
<email address hidden>
http://NxNW.org/~steve/

Revision history for this message
Steve Beattie (sbeattie) wrote :

I've created test kernels for intrepid in my ppa at https://launchpad.net/~sbeattie/+archive/ppa based on the previously mentioned upstream commit. Testing locally shows that this does address the issue. I'm working on generating kernels for testing on hardy.

You'll want to follow the instructions at https://help.launchpad.net/Packaging/PPA#Adding%20a%20PPA%20to%20your%20Ubuntu%20repositories for installing from this ppa. The kernel this is based off of is the current kernel in intrepid-proposed.

I've attached the git format-patch generated diff.

Note that this kernel is for testing only. Once this issue is verified, the patch will be forwarded into the update process.

Revision history for this message
Steve Beattie (sbeattie) wrote :

There are now test kernels for hardy in my ppa at https://launchpad.net/~sbeattie/+archive/ppa based off of the same patch. Again, local testing shows that the rapid memory leak in the apparmor code in the unpatched kernel is addressed by this fix.

Feedback is appreciated. You'll want to follow the instructions at https://help.launchpad.net/Packaging/PPA#Adding%20a%20PPA%20to%20your%20Ubuntu%20repositories for installing from this ppa. The kernel this is based off of is the current kernel in hardy-proposed. Thanks.

Steve Beattie (sbeattie)
description: updated
Changed in linux:
importance: Undecided → Medium
status: New → Triaged
importance: Undecided → Medium
status: New → Triaged
importance: Undecided → Medium
status: New → Triaged
Revision history for this message
Tim Gardner (timg-tpi) wrote :
Changed in linux:
assignee: nobody → timg-tpi
milestone: none → jaunty-alpha-6
status: Triaged → Fix Committed
Andy Whitcroft (apw)
Changed in linux:
assignee: nobody → apw
importance: Medium → Undecided
status: Triaged → In Progress
assignee: nobody → apw
status: Triaged → In Progress
Revision history for this message
Domas Mituzas (domas-mituzas) wrote :

It fixed at least for my VM tests (though they were mostly same as initial testcase).
Thanks Steve.

Andy Whitcroft (apw)
description: updated
Andy Whitcroft (apw)
Changed in linux:
status: In Progress → Fix Committed
status: In Progress → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package linux - 2.6.28-8.27

---------------
linux (2.6.28-8.27) jaunty; urgency=low

  [ Amit Kucheria ]

  * Updating configs (arm:ixp4xx)

  [ Andy Whitcroft ]

  * SAUCE: enable Intel HDMI output

  [ Manoj Iyer ]

  * SAUCE: Added quirk for Linksys WUSB600N USB wifi-n networking adapter
    - LP: #323473

  [ Steve Beattie ]

  * fix apparmor memory leak on unlinked file ops
    - LP: #329489

  [ Tim Gardner ]

  * SAUCE: Dell XPS710 reboot quirk
    - LP: #323592
  * SAUCE: (drop after 2.6.28) ieee80211: Add infrastructure to obsolete
    scan results
    - LP: #336055
  * Add modules.order to the linux-image package.

  [ Upstream Kernel Changes ]

  * iwlwifi: fix time interval misuse in iwl_poll_{direct_}bit
  * x86: only scan the root bus in early PCI quirks
    - LP: #267295
  * ALSA: hda - Intel HDMI audio support
  * ALSA: hda - Fix unused function in patch_intelhdmi.c
  * ALSA: handle SiI1392 HDMI codec in patch_intelhdmi.c
  * ALSA: hda-intel: reorder HDMI audio enabling sequence
  * ALSA: introduce snd_print_pcm_rates()
  * ALSA: create hda_eld.c for ELD routines and proc interface
  * ALSA: ELD proc interface for HDMI sinks
  * ALSA: hda: make standalone hdmi_fill_audio_infoframe()
  * ALSA: hda: make global snd_print_channel_allocation()
  * ALSA: hda: HDMI channel allocations for audio infoframe
  * ALSA: hda: HDMI channel mapping cleanups
  * ALSA: hda: minor code cleanups
  * ALSA: hda: rename sink_eld to hdmi_eld
  * ALSA: hda - Release ELD proc file
  * ALSA: hda - minor HDMI code cleanups
  * ALSA: hda - report selected CA index for Audio InfoFrame
  * ALSA: hda - Add Intel vendor id string

 -- Tim Gardner <email address hidden> Wed, 25 Feb 2009 14:23:46 -0700

Changed in linux:
status: Fix Committed → Fix Released
Revision history for this message
Steve Beattie (sbeattie) wrote :

This bug is in the kernel portion of apparmor, closing the userspace apparmor component tasks. Sorry for the noise.

Changed in apparmor:
status: Confirmed → Invalid
status: Confirmed → Invalid
status: Confirmed → Invalid
Revision history for this message
Martin Pitt (pitti) wrote :

Accepted intrepid into linux-proposed; please test and give feedback here. Please see https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Revision history for this message
Martin Pitt (pitti) wrote :

Accepted linux into hardy-proposed; please test and give feedback here. Please see https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package linux - 2.6.27-11.31

---------------
linux (2.6.27-11.31) intrepid-security; urgency=low

  [ Steve Beattie ]

  * fix apparmor memory leak on deleted file ops Bug: #329489
    - LP: #329489

  [ Upstream Kernel Changes ]

  * sctp: Avoid memory overflow while FWD-TSN chunk is received with bad
    stream ID
    - CVE-2009-0065
  * prevent kprobes from catching spurious page faults
    - CVE-2009-0605
  * net: 4 bytes kernel memory disclosure in SO_BSDCOMPAT gsopt try #2
    - CVE-2009-0676
  * copy_process: fix CLONE_PARENT && parent_exec_id interaction
    - CVE-2009-0028
  * security: introduce missing kfree
    - CVE-2009-0031
  * eCryptfs: check readlink result was not an error before using it
    - CVE-2009-0269
  * dell_rbu: use scnprintf() instead of less secure sprintf()
    - CVE-2009-0322
  * drivers/net/skfp: if !capable(CAP_NET_ADMIN): inverted logic
    - CVE-2009-0675
  * ext4: Initialize the new group descriptor when resizing the filesystem
    - CVE-2009-0745
  * ext4: Add sanity check to make_indexed_dir
    - CVE-2009-0746
  * ext4: only use i_size_high for regular files
    - CVE-2009-0747
  * ext4: Add sanity checks for the superblock before mounting the
    filesystem
    - CVE-2009-0748
  * x86-64: syscall-audit: fix 32/64 syscall hole
    - CVE-2009-0834
  * x86-64: seccomp: fix 32/64 syscall hole
    - CVE-2009-0835
  * shm: fix shmctl(SHM_INFO) lockup with !CONFIG_SHMEM
    - CVE-2009-0859
  * udf:SAUCE (drop after 2.6.30): Fix oops when invalid character in
    filename occurs
    - LP: #321606
  * Add '-fwrapv' to gcc CFLAGS
    - LP: #348015
  * Move cc-option to below arch-specific setup
    - LP: #348015
  * Fix memory corruption in console selection
    - CVE-2009-1046

 -- Stefan Bader <email address hidden> Mon, 16 Mar 2009 18:48:27 +0100

Changed in linux (Ubuntu Intrepid):
status: Fix Committed → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package linux - 2.6.24-23.52

---------------
linux (2.6.24-23.52) hardy-security; urgency=low

  [Stefan Bader]

  * rt: Fix FTBS caused by shm changes
    - CVE-2009-0859

  [Steve Beattie]

  * fix apparmor memory leak on deleted file ops Bug: #329489
    - LP: #329489

  [Upstream Kernel Changes]

  * NFS: Remove the buggy lock-if-signalled case from do_setlk()
    - CVE-2008-4307
  * sctp: Avoid memory overflow while FWD-TSN chunk is received with bad
    stream ID
    - CVE-2009-0065
  * net: 4 bytes kernel memory disclosure in SO_BSDCOMPAT gsopt try #2
    - CVE-2009-0676
  * sparc: Fix mremap address range validation.
    - CVE-2008-6107
  * copy_process: fix CLONE_PARENT && parent_exec_id interaction
    - CVE-2009-0028
  * security: introduce missing kfree
    - CVE-2009-0031
  * eCryptfs: check readlink result was not an error before using it
    - CVE-2009-0269
  * dell_rbu: use scnprintf() instead of less secure sprintf()
    - CVE-2009-0322
  * drivers/net/skfp: if !capable(CAP_NET_ADMIN): inverted logic
    - CVE-2009-0675
  * Ext4: Fix online resize block group descriptor corruption
    - CVE-2009-0745
  * ext4: Initialize the new group descriptor when resizing the filesystem
    - CVE-2009-0745
  * ext4: Add sanity check to make_indexed_dir
    - CVE-2009-0746
  * x86-64: syscall-audit: fix 32/64 syscall hole
    - CVE-2009-0834
  * x86-64: seccomp: fix 32/64 syscall hole
    - CVE-2009-0835
  * shm: fix shmctl(SHM_INFO) lockup with !CONFIG_SHMEM
    - CVE-2009-0859
  * apparmor: Fix handling of larger number of profiles
    - LP: #345144
  * udf:SAUCE (drop after 2.6.30): Fix oops when invalid character in
    filename occurs
    - LP: #321606
  * Fix memory corruption in console selection
    - CVE-2009-1046
  * SPARC64: Loosen checks in exception table handling.
    - LP: #301608, #349655

 -- Stefan Bader <email address hidden> Mon, 16 Mar 2009 18:39:14 +0100

Changed in linux (Ubuntu Hardy):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.