On 10/05/2016 07:36 AM, Tyler Hicks wrote: > On 10/05/2016 02:46 AM, John Johansen wrote: >> On 10/04/2016 07:32 PM, Tyler Hicks wrote: >>> On 10/04/2016 06:31 PM, John Johansen wrote: >>>> exec_stack picked up a fix to address a semantic change introduced in >>>> 4.8 kernels. However this breaks the exec_stack test for kernel pre >>>> 4.8. This patch uses an apparmor kernel flag to detect whether the >>>> semantic change is present and adjusts the test accordingly. >>> >>> A couple questions below... >>> >>>> >>>> --- >>>> >>>> === modified file 'tests/regression/apparmor/exec_stack.sh' >>>> --- tests/regression/apparmor/exec_stack.sh 2016-09-29 04:11:29 >>>> +0000 >>>> +++ tests/regression/apparmor/exec_stack.sh 2016-10-04 21:15:48 >>>> +0000 >>>> @@ -43,6 +43,12 @@ >>>> >>>> touch $file $otherfile $sharedfile $thirdfile >>>> >>>> +if [ "$(kernel_features domain/fix_binfmt_elf_mmap)" == "true" ]; then >>> >>> Why is the kernel doing domain/fix_binfmt_elf_mmap instead of bumping >>> the kABI? Maybe I'm misunderstanding the purpose of the kABI but I >>> understood it to be bumped when there were was a change in mediation >>> that causes policy change. >>> >> >> For a few reasons. >> 1. I brought up bumping the potential of bumping the abi and got no >> feedback. > > Sorry about that. I'm still trying to fully understand how kABI is > intended to be used (I think I'm getting there now) so I don't know that > I could have provided much feedback. > >> 2. The abi bump here is odd in that it is kernel caused not apparmor. >> That doesn't mean we don't bump the abi but it is less clear. Hence 1 >> 3. I am worried that the patch that introduced the semantic change >> will show up in stable kernels as it is an information leak fix >> for tracing. For none ubuntu kernels this will mean a different >> abi on the apparmor side, so bumping the abi alone is problematic. >> Ubuntu is at v7, while upstream is at v6. We can bump the abi to v8 >> but what do we do for upstream or other distro kernels? We can't >> bump them to v7, nor v8 as they are missing other semantic changes. > > Interesting and frustrating problem. > >> >> This is a case where a sub version would be nice. >> >> so with that issue in place so it can be properly detected and conditioned >> in the regression tests. Since we can't currently tie it to a single >> abi number. >> >> With that said I am still open to bumping to v8 for stacking based >> kernels. But we need the flag to deal with v6 abi based kernels if >> this ever happens. >> >> I am no also open to the idea of using one or two bits of the abi >> for a sub version. So we can handle something like this better in the >> future. > > One or two bits doesn't seem like enough. If that's all that we have to > spare, I think I would have chosen 'domain/fix_binfmt_elf_mmap', too. > >> >>>> + elfmmap="m" >>>> +else >>>> + elfmmap="" >>>> +fi >>>> + >>>> # Verify file access and contexts by an unconfined process >>>> runchecktest "EXEC_STACK (unconfined - file)" pass -f $file >>>> runchecktest "EXEC_STACK (unconfined - otherfile)" pass -f $otherfile >>>> @@ -66,7 +72,7 @@ >>>> >>>> # Verify file access and contexts by 2 stacked profiles >>>> genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \ >>>> - image=$othertest addimage:$test $otherok $sharedok $getcon $test:rm >>>> + image=$othertest addimage:$test $otherok $sharedok $getcon >>>> $test:r$elfmmap >>> >>> The previous change (r3509) simply added 'm' to the existing '$test r,' >>> rules but this patch description says, "this breaks the exec_stack test >>> for kernel pre 4.8." Is it true that adding 'm' actually broke the tests >>> in pre 4.8 or are you just trying to make the tests more accurate? >>> >> In this case yes. I originally was worried about it breaking over looking >> that its just an addition. But even so these are the only tests we have >> that caught the behavioral change, and I didn't want to loose that. > > I asked which of two scenarios are true and you answered "yes". :) > yes. They are both true, initially I did it because I was worried about breaking old kernels, then it clicked.
I then wanted it to keep them for accuracy and a fallback for testing until we made a test specifically for this. > I assume that you were answering yes to "are you just trying to make the > tests more accurate?". > > My concern was that if adding 'm' to the target profile broke things, we > have a larger problem on our hands in that if we update any profiles to > add 'm' to the target profile, as required by 4.8 and newer kernels, > then those profiles wouldn't work on older kernels any longer. I wanted > to confirm that wasn't the case. > It isn't. It is just an extra permission. However we have that problem on the other side as well, where the m is needed on the calling profile for pre 4.8 kernels, and not for 4.8 kernels. So to have policy that works for both we now require the m in both the calling and transition profile. >> >> Yes we need an separate test for this semantic, but its what I had because >> I had done this based off of my original unfounded worries, and I wanted >> to get it out asap. Partly because even in ubuntu we have users using >> custom kernels, old kernels, backport kernels, and again I am worried >> about the semantic change patch showing up and breaking things, leading >> to bug reports, and time wasted. >> >> So yes we can withdraw this patch once we have a dedicated test. > > No, I think that this patch is fine and I didn't meant to hold you up on > committing it. I was only curious about the bigger questions that this > patch raised. Please add my ack and commit ASAP. > thanks I will update the description to make it less confusing > Acked-by: Tyler Hicks <[email protected]> > > Thanks! > > Tyler > >> >> >>> Tyler >>> >>>> runchecktest_errno EACCES "EXEC_STACK (2 stacked - file)" fail -- $test >>>> -f $file >>>> runchecktest_errno EACCES "EXEC_STACK (2 stacked - otherfile)" fail -- >>>> $test -f $otherfile >>>> runchecktest_errno EACCES "EXEC_STACK (2 stacked - thirdfile)" fail -- >>>> $test -f $thirdfile >>>> @@ -79,7 +85,7 @@ >>>> # Verify file access and contexts by 3 stacked profiles >>>> genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \ >>>> image=$othertest addimage:$test $otherok $sharedok $getcon $test:"rix >>>> -> &$thirdtest" -- \ >>>> - image=$thirdtest addimage:$test $thirdok $sharedok $getcon $test:rm >>>> + image=$thirdtest addimage:$test $thirdok $sharedok $getcon >>>> $test:r$elfmmap >>>> runchecktest_errno EACCES "EXEC_STACK (3 stacked - file)" fail -- $test >>>> -- $test -f $file >>>> runchecktest_errno EACCES "EXEC_STACK (3 stacked - otherfile)" fail -- >>>> $test -- $test -f $otherfile >>>> runchecktest_errno EACCES "EXEC_STACK (3 stacked - thirdfile)" fail -- >>>> $test -- $test -f $thirdfile >>>> @@ -89,7 +95,7 @@ >>>> >>>> genprofile -I $sharedok $stackotherok $stackthirdok $test:"rix -> >>>> &$othertest" -- \ >>>> image=$othertest addimage:$test $sharedok $stackthirdok $test:"rix -> >>>> &$thirdtest" -- \ >>>> - image=$thirdtest addimage:$test $sharedok $stackthirdok $test:rm >>>> + image=$thirdtest addimage:$test $sharedok $stackthirdok $test:r$elfmmap >>>> # Triggered an AppArmor WARN in the initial stacking patch set >>>> runchecktest "EXEC_STACK (3 stacked - old AA WARN)" pass -p $othertest -- >>>> $test -p $thirdtest -f $sharedfile >>>> >>>> @@ -120,7 +126,7 @@ >>>> >>>> # Verify file access and contexts in mixed mode >>>> genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \ >>>> - image=$othertest flag:complain addimage:$test $otherok $sharedok >>>> $getcon $test:rm >>>> + image=$othertest flag:complain addimage:$test $otherok $sharedok >>>> $getcon $test:r$elfmmap >>>> runchecktest "EXEC_STACK (mixed mode - file)" pass -- $test -f $file >>>> runchecktest_errno EACCES "EXEC_STACK (mixed mode - otherfile)" fail -- >>>> $test -f $otherfile >>>> runchecktest "EXEC_STACK (mixed mode - sharedfile)" pass -- $test -f >>>> $sharedfile >>>> >>>> >>>> >>> >>> >> > > -- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
