Message ID | e2a2fe02-0baf-ecac-9bcc-050e53003925@canonical.com |
---|---|
State | New |
Headers | show |
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.
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;
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>