[2.6.39-rc2, framebuffer] use after free oops

When building in the eg vga16 and intelfb framebuffers and page
debugging, we can get hit by a use-after-free oops [1]; with page
debugging disabled, this slips by unnoticed.

When intelfb registers, the vga16 framebuffer is detected covering
it's I/O regions, so it is removed (in
remove_conflicting_framebuffers->unregister_framebuffer). The problem
is the fb_info structure is freed (in
unregister_framebuffer->vga16fb_destroy->framebuffer_release->kfree)
before all open files to it are closed, so we get a use-after-free
scenario.

Freeing should be deferred until the last user has gone away. What
approach would make sense here?

Daniel

--- [1]

checking generic (a0000 10000) vs hw (d0000000 10000000)
fb: conflicting fb hw usage inteldrmfb vs VGA16 VGA - removing generic driver
Console: switching to colour VGA+ 80x25
BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
IP: [<ffffffff81708394>] mutex_lock_nested+0x94/0x390
PGD 2203ab067 PUD 2203ac067 PMD 0
Oops: 0002 [#2] PREEMPT SMP DEBUG_PAGEALLOC
last sysfs file: /sys/devices/pci0000:00/0000:00:1b.0/sound/card0/uevent
CPU 3
Modules linked in: arc4 ecb uvcvideo videodev v4l2_compat_ioctl32
i915(+) drm_kms_helper iwlagn mmc_block mac80211 drm video sdhci_pci
sdhci mmc_core

Pid: 271, comm: plymouthd Tainted: G      D     2.6.39-rc2-350cd+ #1
Dell Inc. Latitude E5420/0H5TG2
RIP: 0010:[<ffffffff81708394>]  [<ffffffff81708394>]
mutex_lock_nested+0x94/0x390
RSP: 0018:ffff88021ee77c08  EFLAGS: 00010046
RAX: 0000000000010000 RBX: 0000000000000008 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffff8170838b
RBP: ffff88021ee77c78 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff88021e574020
R13: 0000000000000010 R14: 0000000000000246 R15: ffff88021de6f478
FS:  00007f50d6e9c720(0000) GS:ffff88022ec60000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000010 CR3: 00000002203aa000 CR4: 00000000000406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process plymouthd (pid: 271, threadinfo ffff88021ee76000, task ffff88021e574020)
Stack:
 ffffffff81375552 0000000000000000 00000000fffffff2 ffff88021de6f478
 ffff88021e574020 0000000000000246 0000000000000000 00000000fffffff2
 ffff88021ee77c68 0000000000000000 0000000000000008 0000000000000000
Call Trace:
 [<ffffffff81375552>] ? lock_fb_info+0x22/0x60
 [<ffffffff81375552>] lock_fb_info+0x22/0x60
 [<ffffffff8137a24b>] fb_set_user_cmap+0x15b/0x1a0
 [<ffffffff81376e28>] do_fb_ioctl+0x4b8/0x5e0
 [<ffffffff81105dae>] ? might_fault+0x4e/0xa0
 [<ffffffff81058e61>] ? get_parent_ip+0x11/0x50

22 more lines

Any ideas on how best to address this issue [0], since it causes
silent corruption, or at best crashes?

Thanks,
  Daniel

--- [0]

When building in the eg vga16 and intelfb framebuffers and page
debugging, we can get hit by a use-after-free oops [1]; with page
debugging disabled, this slips by unnoticed.

When intelfb registers, the vga16 framebuffer is detected covering
it's I/O regions, so it is removed (in
remove_conflicting_framebuffers->unregister_framebuffer). The problem
is the fb_info structure is freed (in
unregister_framebuffer->vga16fb_destroy->framebuffer_release->kfree)
before all open files to it are closed, so we get a use-after-free
scenario.

Freeing should be deferred until the last user has gone away. What
approach would make sense here?

Daniel

--- [1]

checking generic (a0000 10000) vs hw (d0000000 10000000)
fb: conflicting fb hw usage inteldrmfb vs VGA16 VGA - removing generic driver
Console: switching to colour VGA+ 80x25
BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
IP: [<ffffffff81708394>] mutex_lock_nested+0x94/0x390
PGD 2203ab067 PUD 2203ac067 PMD 0
Oops: 0002 [#2] PREEMPT SMP DEBUG_PAGEALLOC
last sysfs file: /sys/devices/pci0000:00/0000:00:1b.0/sound/card0/uevent
CPU 3
Modules linked in: arc4 ecb uvcvideo videodev v4l2_compat_ioctl32
i915(+) drm_kms_helper iwlagn mmc_block mac80211 drm video sdhci_pci
sdhci mmc_core

Pid: 271, comm: plymouthd Tainted: G      D     2.6.39-rc2-350cd+ #1
Dell Inc. Latitude E5420/0H5TG2
RIP: 0010:[<ffffffff81708394>]  [<ffffffff81708394>]
mutex_lock_nested+0x94/0x390
RSP: 0018:ffff88021ee77c08  EFLAGS: 00010046
RAX: 0000000000010000 RBX: 0000000000000008 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffff8170838b
RBP: ffff88021ee77c78 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff88021e574020
R13: 0000000000000010 R14: 0000000000000246 R15: ffff88021de6f478
FS:  00007f50d6e9c720(0000) GS:ffff88022ec60000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000010 CR3: 00000002203aa000 CR4: 00000000000406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process plymouthd (pid: 271, threadinfo ffff88021ee76000, task ffff88021e574020)
Stack:
 ffffffff81375552 0000000000000000 00000000fffffff2 ffff88021de6f478
 ffff88021e574020 0000000000000246 0000000000000000 00000000fffffff2
 ffff88021ee77c68 0000000000000000 0000000000000008 0000000000000000
Call Trace:
 [<ffffffff81375552>] ? lock_fb_info+0x22/0x60
 [<ffffffff81375552>] lock_fb_info+0x22/0x60
 [<ffffffff8137a24b>] fb_set_user_cmap+0x15b/0x1a0
 [<ffffffff81376e28>] do_fb_ioctl+0x4b8/0x5e0
 [<ffffffff81105dae>] ? might_fault+0x4e/0xa0
 [<ffffffff81058e61>] ? get_parent_ip+0x11/0x50

27 more lines

On Wed, 20 Apr 2011 13:50:10 Daniel J Blueman [off-list ref] wrote:

Any ideas on how best to address this issue [0], since it causes
silent corruption, or at best crashes?

There is probably no easy short-term fix to this... The complex fix
would be to implement some deferred free with driver-local ref-counting
though to be complete it would have to be done for pretty much every FB
driver!

The mid-term fix would be to have fb_info ref-counted by FB
subsystem (I'm working on it, slowly). This way freeing would be
deferred to when last references is being dropped.
The driver then just has to know when it can't touch underlying
hardware anymore (and do the right thing with framebuffer memory in
case it is still mmapped somewhere).

Bruno

49 more quoted lines

On Wed, 20 Apr 2011 08:05:35 +0200
Bruno Prémont [off-list ref] wrote:

On Wed, 20 Apr 2011 13:50:10 Daniel J Blueman [off-list ref] wrote:

quoted

Any ideas on how best to address this issue [0], since it causes
silent corruption, or at best crashes?

There is probably no easy short-term fix to this... 

The short term fix would be to deliberately leak the buffer. That should
go into 2.6.39-rc right now with a comment explaining the situation.
Otherwise who knows what corruption may occur to user data if unlucky.

The other 'cheat' might be to tweak the API so the removal API isn't a
'destroy' interface but a 'shut down' and has a matching 'restart' one
for when the intelfb unloads at which point vga16fb can carry on with the
original fb_info 8)

Alan

On 20 April 2011 17:56, Alan Cox [off-list ref] wrote:

On Wed, 20 Apr 2011 08:05:35 +0200
Bruno Prémont [off-list ref] wrote:

quoted

On Wed, 20 Apr 2011 13:50:10 Daniel J Blueman [off-list ref] wrote:

quoted

Any ideas on how best to address this issue [0], since it causes
silent corruption, or at best crashes?

There is probably no easy short-term fix to this...

The short term fix would be to deliberately leak the buffer. That should
go into 2.6.39-rc right now with a comment explaining the situation.
Otherwise who knows what corruption may occur to user data if unlucky.

The other 'cheat' might be to tweak the API so the removal API isn't a
'destroy' interface but a 'shut down' and has a matching 'restart' one
for when the intelfb unloads at which point vga16fb can carry on with the
original fb_info 8)

It looks like Andy Whitcroft addressed this issue some time ago, but
forgot to send the fix upstream:

http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-natty.git;a=patch;hÅa742b5f78e161d6a13853a7e3e6e1dfa429e69;hp&a1443f67eea17d4b78ef75df701782cc8bf35b

Let's hope it can hit -rc7 since it's been in Ubuntu's kernel tree for
considerable time, and fixes a silent corrupter:

http://groups.google.com/group/linux.kernel/browse_thread/thread/fc9083f6f380ed5b/f801112b840785cb?show_docidø01112b840785cb

Thanks,
  Daniel
-- 
Daniel J Blueman

Hi, Daniel J Blueman.

Did you test https://lkml.org/lkml/2011/5/5/208 ? And it works for you ?
Then please reply with your error and an Tested-by.

And CC: "Dave Airlie" [off-list ref]

On 7 May 2011 23:24, Anca Emanuel [off-list ref] wrote:

Hi, Daniel J Blueman.

Did you test https://lkml.org/lkml/2011/5/5/208 ? And it works for you ?
Then please reply with your error and an Tested-by.

And CC: "Dave Airlie" [off-list ref]

Tested against 2.6.39-rc6. Instrumentation and debug catches the
(silent without debug) use-after-free case, which now doesn't show up
with this patch, so looks good. Probably good sense to get into
-stable too.

Tested-by: Daniel J Blueman <redacted>

Thanks,
  Daniel
-- 
Daniel J Blueman