Regression tests cannot write to apparmor path_max module parameter in artful/4.11

Bug #1692543 reported by Seth Forshee
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
apparmor (Ubuntu)
New
Undecided
Unassigned

Bug Description

The longpath regression tests tries to write to /sys/module/apparmor/parameters/path_max, but this is read-only in artful/4.11:

commit cdc8e09e16bb7eb7d23fcbdbe416aa91770fb4d6
Author: John Johansen <email address hidden>
Date: Thu Apr 6 05:14:20 2017 -0700

    apparmor: Make path_max parameter readonly

This is causing ADT to fail.

https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-artful-canonical-kernel-team-ppa/artful/amd64/l/linux/20170519_170900_9130b@/log.gz

  running longpath
  longpath.sh: line 53: /sys/module/apparmor/parameters/path_max: Permission denied
  Fatal Error (longpath): Unexpected shell error. Run with -x to debug

Tags: patch
Revision history for this message
John Johansen (jjohansen) wrote :

This is caused do a change made upstream in the 4.11 kernel, which forbids writing the buffer size parameter after boot. The change to boot time preallocated work buffers made this parameter useless, but 4.11 only partially merged that work, making writing the buffer size an attack vector on the kernel memory and with user namespaces enabled any user could exploit it. Hence a follow-on patch to make the buffer size read-only after boot.

We have 2 choices for dealing with this, either fix the regression tests OR we can virtualize the buffer size parameter per namespace, capping the virtualized size by what was allocated at boot.

Revision history for this message
Seth Forshee (sforshee) wrote :

What if the test was changed to check writability of path_max? Just changing the the check for the sysfs path to -w won't work for root, but maybe something like this (perhaps there's a simpler way to do the check though):

if [[ -f /sys/module/apparmor/parameters/path_max &&
      $(stat -c "%a" /sys/module/apparmor/parameters/path_max) == "600" ]] ; then
        buf_max=2048 #standard x86 page size/2
        old_max=`cat /sys/module/apparmor/parameters/path_max`
        echo $buf_max > /sys/module/apparmor/parameters/path_max
else
        echo "WARNING: This version of AppArmor does not support changing buffer size."
fi

Revision history for this message
John Johansen (jjohansen) wrote :

yes something like this should work. However 600 will not be the correct check, as in some cases the owner may differ, especially in the virtualized case because vfs doesn't let us virtualize the file's owner.

Currently this file isn't virtualized to the poilicy namespace, and that is why the restriction was put in place to keep containers from doing things they shouldn't with other policy namespaces. It will be virtualized by 4.14, we can land that change in Ubuntu for 17.10 and SRU.

We have no plans to require/use bind mounts to get around owner virtualization, but with the 4.13 we added the ability for securityfs to do magic symlinks, which allows a virtualization like nsfs is doing. We could make this a magic symlink to a file owned by the correct user if need be, in which case 600 would be sufficient.

I think for now the test needs to be for any possible write, 222, so we will need to do some masking on the returned value.

Revision history for this message
Seth Forshee (sforshee) wrote :

Something more like this then.

if [ ! -f /sys/module/apparmor/parameters/path_max ] ; then
 echo "WARNING: This version of AppArmor does not support changing buffer size."
else
 mode=$(stat -c "%a" /sys/module/apparmor/parameters/path_max)
 if (( 8#$mode & 0222 )); then
  buf_max=2048 #standard x86 page size/2
  old_max=`cat /sys/module/apparmor/parameters/path_max`
  echo $buf_max > /sys/module/apparmor/parameters/path_max
 else
  echo "WARNING: This version of AppArmor does not support changing buffer size."
 fi
fi

Revision history for this message
Seth Forshee (sforshee) wrote :

This patch fixes the tests for me. Turns out that we can't simply skip writing to path_max as that seems to break the test case completely, so I changed it to print an XFAIL message and abort the test.

tags: added: patch
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

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