[xenial,yakkety] UBUNTU: SAUCE: apparmor: fix sleep in critical section

UBUNTU: SAUCE: apparmor: fix sleep in critical section

path_put() call dput() which might sleep on some paths. When it does
sleep from these code paths, the per cpu work buffer may get reused
overwriting the data that was just placed in the buffer.

This causes the following mediation to fail as the work buffer no
longer has valid data for the current operation.

BugLink: http://bugs.launchpad.net/bugs/1634753
Signed-off-by: John Johansen <john.johansen@canonical.com>

Looks to be doing what it claims and releases the path reference only after
buffer references have been released. That also better matches the order things
get acquired.

On Wed, Oct 19, 2016 at 08:17:20AM +0200, John Johansen wrote:
> UBUNTU: SAUCE: apparmor: fix sleep in critical section
> 
> path_put() call dput() which might sleep on some paths. When it does
> sleep from these code paths, the per cpu work buffer may get reused
> overwriting the data that was just placed in the buffer.
> 
> This causes the following mediation to fail as the work buffer no
> longer has valid data for the current operation.
> 
> BugLink: http://bugs.launchpad.net/bugs/1634753
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> 
> diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
> index b380e32..ee07f76 100644
> --- a/security/apparmor/mount.c
> +++ b/security/apparmor/mount.c
> @@ -405,7 +405,6 @@ int aa_bind_mount(struct aa_label *label, struct path *path,
>  						   &old_path),
>  			     old_buffer, &old_name, &info,
>  			     labels_profile(label)->disconnected);
> -	path_put(&old_path);
>  	if (error)
>  		goto error;
>  
> @@ -415,6 +414,7 @@ int aa_bind_mount(struct aa_label *label, struct path *path,
>  
>  out:
>  	put_buffers(buffer, old_buffer);
> +	path_put(&old_path);
>  
>  	return error;
>  
> @@ -494,7 +494,6 @@ int aa_move_mount(struct aa_label *label, struct path *path,
>  						   &old_path),
>  			     old_buffer, &old_name, &info,
>  			     labels_profile(label)->disconnected);
> -	path_put(&old_path);
>  	if (error)
>  		goto error;
>  
> @@ -504,6 +503,7 @@ int aa_move_mount(struct aa_label *label, struct path *path,
>  
>  out:
>  	put_buffers(buffer, old_buffer);
> +	path_put(&old_path);
>  
>  	return error;
>  
> @@ -557,7 +557,6 @@ int aa_new_mount(struct aa_label *label, const char *orig_dev_name,
>  						&dev_path),
>  				     dev_buffer, &dev_name, &info,
>  				     labels_profile(label)->disconnected);
> -		path_put(&dev_path);
>  		if (error)
>  			goto error;
>  	}
> @@ -574,6 +573,8 @@ int aa_new_mount(struct aa_label *label, const char *orig_dev_name,
>  
>  cleanup:
>  	put_buffers(buffer, dev_buffer);
> +	if (requires_dev)
> +		path_put(&dev_path);
>  
>  	return error;
>  

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw

Hello.

John Johansen wrote:
> UBUNTU: SAUCE: apparmor: fix sleep in critical section
> 
> path_put() call dput() which might sleep on some paths. When it does
> sleep from these code paths, the per cpu work buffer may get reused
> overwriting the data that was just placed in the buffer.

Excuse me, but I just wondered how path_put() can be called from a
critical section. OK, get_buffers() calls preempt_disable() and
put_buffers() calls preempt_enable(). Then, I think that calling
kern_path() from between get_buffers() and put_buffers() is also wrong
because it does a GFP_KERNEL allocation which can sleep.
Did you already fix it?

On 10/19/2016 12:58 PM, Tetsuo Handa wrote:
> Hello.
> 
> John Johansen wrote:
>> UBUNTU: SAUCE: apparmor: fix sleep in critical section
>>
>> path_put() call dput() which might sleep on some paths. When it does
>> sleep from these code paths, the per cpu work buffer may get reused
>> overwriting the data that was just placed in the buffer.
> 
> Excuse me, but I just wondered how path_put() can be called from a
> critical section. OK, get_buffers() calls preempt_disable() and
It shouldn't be, that is what the patch fixes. And yes get_buffers starts
a critical section

> put_buffers() calls preempt_enable(). Then, I think that calling
> kern_path() from between get_buffers() and put_buffers() is also wrong
> because it does a GFP_KERNEL allocation which can sleep.
> Did you already fix it?
> 
kern_path() is not called in the get_buffers()/put_buffers() critical
section on the xenial which this patch is for

other patches for other kernel versions will follow

John - this patch appears to have already been applied in another form
to both Xenial and Yakkety. What gives ?

On 10/28/2016 06:09 AM, Tim Gardner wrote:
> John - this patch appears to have already been applied in another form
> to both Xenial and Yakkety. What gives ?
> 
> 
Tim,

sorry it should have been tagged as Xenial only, and it specifically was targeted at last weeks CVE kernel (against -updates last week) for the snappy release. The Yakkety version of apparmor and the Xenial SRU (which was in -proposed) of it already have the fix, which is what you are seeing now.