[apparmor] [patch] - fix exec_stack to work on pre 4.8 kernels

[Tyler Hicks]

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". :)

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.

> 
> 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.

  Acked-by: Tyler Hicks <tyhicks at canonical.com>

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
>>>
>>>
>>>
>>
>>
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20161005/63f4b588/attachment.pgp>