Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

of: Fix crash in of_build_overlay_info #164

Open
wants to merge 121 commits into
base: 4.14
Choose a base branch
from

Conversation

jharvell
Copy link

@jharvell jharvell commented May 6, 2018

Do not set of_overlay_info attribute_group name from overlay info when there is no
overlay info device

Analysis of kernel stack trace, registers and stack along with disassembly of code pointed to ovinfo->info being zero at this point in the code.

Application of this patch prevents crash though overlays fail to load for various different reasons.

@jharvell
Copy link
Author

jharvell commented May 6, 2018

In case someone is interested in reproducing the issue, I saw the crash with several overlays I tried to load. One of these is univ-bbgw-00A0.dtbo. I built this from sources below:

joey@akita arm$ pwd
/home/joey/git/bb.org-overlays/src/arm
joey@akita arm$ git remote -v
origin  [email protected]:beagleboard/bb.org-overlays.git (fetch)
origin  [email protected]:beagleboard/bb.org-overlays.git (push)
joey@akita arm$ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
joey@akita arm$ git --no-pager log -n1
commit 604c0926a4f7505dfc3d501301413c821e59febe (HEAD -> master, origin/master, origin/HEAD)
Author: Robert Nelson <[email protected]>
Date:   Tue Apr 24 08:41:58 2018 -0500

    universal rewrite: spi0-xyz and spidev1x
    
    Signed-off-by: Robert Nelson <[email protected]>

Before the fix in this PR, echoing univ-bbgw to /sys/devices/platform/bone_capemgr/slots resulted i a SEGV with the kernel debug info in dmesg pointing to ovinfo->info being NULL.

After the fix, I see the following when trying to apply the same overlay:

pip /lib/firmware # echo univ-bbgw > /sys/devices/platform/bone_capemgr/slots 
bash: echo: write error: Invalid argument

[  290.750632] bone_capemgr bone_capemgr: part_number 'univ-bbgw', version 'N/A'
[  290.758437] bone_capemgr bone_capemgr: slot #4: override
[  290.768093] bone_capemgr bone_capemgr: Using override eeprom data at slot 4
[  290.780720] bone_capemgr bone_capemgr: slot #4: 'Override Board Name,00A0,Override Manuf,univ-bbgw'
[  290.847781] OF: overlay: Failed to apply prop @/__symbols__/pruss
[  290.854191] OF: overlay: apply failed '/__symbols__'
[  290.863083] bone_capemgr bone_capemgr: slot #4: Failed to create overlay

@jharvell
Copy link
Author

jharvell commented May 7, 2018

I did a little more analysis. This crash will occur for any device blob that has a node /symbols. See the code below. An entry in ovinfo is added for each child node of tree in the first loop. ovinfo[cnt].info is populated in that case. Then there is a search for child node with name symbols. If found, it is added as the last element ovinfo. But for this case, ovinf[cnt].info is not set. And this is what the code crashes on. So if the compiler puts a /symbols node in the tree, then the kernel will crash tyring to apply it.

I made a simple overlay with one node, and compiled it. When I do strings on that blob, there is no "symbols" string in the blob and that does not trigger this bug. Several other blobs I have tried from bb.org-overlays blobs have symbols in them and trigger this crash.

	cnt = 0;
	for_each_child_of_node(tree, node) {
		err = of_fill_overlay_info(ov, node, &ovinfo[cnt]);
		if (err == 0)
			cnt++;
	}

	node = of_get_child_by_name(tree, "__symbols__");
	if (node) {
		ovinfo[cnt].overlay = node;
		ovinfo[cnt].target = of_find_node_by_path("/__symbols__");
		ovinfo[cnt].is_symbols_node = 1;

		if (!ovinfo[cnt].target) {
			pr_err("no symbols in root of device tree.\n");
			return -EINVAL;
		}

		cnt++;
	}

RobertCNelson and others added 28 commits May 7, 2018 16:08
Signed-off-by: Robert Nelson <[email protected]>
Signed-off-by: Robert Nelson <[email protected]>
Signed-off-by: Robert Nelson <[email protected]>
Signed-off-by: Robert Nelson <[email protected]>
Signed-off-by: Robert Nelson <[email protected]>
Signed-off-by: Robert Nelson <[email protected]>
Signed-off-by: Robert Nelson <[email protected]>
#https://lkml.org/lkml/2016/6/14/967

Pwm channels don't send uevents when exported, this change adds the
channels to a pwm class and set their device type to pwm_channel so
uevents are sent.

To do this properly, the device names need to change to uniquely
identify a channel.  This change is from pwmN to pwm-(chip->base):N

Signed-off-by: David Hsu <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Robert Nelson <[email protected]>
This reverts commit 956b200.

Signed-off-by: Robert Nelson <[email protected]>
ADC channel 0 photodiode detects both infrared + visible light,
but ADC channel 1 just detects infrared. However, the latter is a bit
more sensitive in that range so complete darkness or low light causes
a error condition in which the chan0 - chan1 is negative that
results in a -EAGAIN.

This patch changes the resulting lux1_input sysfs attribute message from
"Resource temporarily unavailable" to a user-grokable lux value of 0.

Signed-off-by: Matt Ranostay <[email protected]>
Signed-off-by: Robert Nelson <[email protected]>
Signed-off-by: Pantelis Antoniou <[email protected]>
When disabling an omap device (not when removing the driver), the
device is removed but the hwmod's linger.

Fix the resource leak and the crash when calling omap_device_idle()
after the device's omap data have been removed.

Signed-off-by: Pantelis Antoniou <[email protected]>
Having an omap serial device without a serial aliases doesn't
work. For now fallback to using the hwmod instance.

Signed-off-by: Pantelis Antoniou <[email protected]>
When using DT the driver devm_kalloc's platform data and assigns them
directly to the pdev->dev.platform variable.

This is wrong since device de-registration expects the data to be
kmalloc'ed instead, resulting in a crash.

Fix by copying the platform data to a kmalloc buffer.

Signed-off-by: Pantelis Antoniou <[email protected]>
We are going to need the overlays to appear on sysfs with runtime
global properties (like master enable) so turn them into kobjects.

They have to be in sysfs so that people can have information about the
overlays applied in the system, i.e. where their targets are and whether
removal is possible. In a future more attributes can be added
in a backwards compatible manner.

Signed-off-by: Pantelis Antoniou <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
A throw once master enable switch to protect against any
further overlay applications if the administrator desires so.

A kernel command line option is provided as well.

Signed-off-by: Pantelis Antoniou <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Documentation ABI entry for overlays sysfs entries.

Signed-off-by: Pantelis Antoniou <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Document the of_overlay_disable parameter.

Signed-off-by: Pantelis Antoniou <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
* A per overlay can_remove sysfs attribute that reports whether
the overlay can be removed or not due to another overlapping overlay.

* A target sysfs attribute listing the target of each fragment,
in a group named after the name of the fragment.

Signed-off-by: Pantelis Antoniou <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Documentation for the per-overlay attributes.

Signed-off-by: Pantelis Antoniou <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
@RobertCNelson RobertCNelson force-pushed the 4.14 branch 2 times, most recently from be56edc to 6ad28ee Compare September 24, 2019 16:34
@RobertCNelson RobertCNelson force-pushed the 4.14 branch 2 times, most recently from 0e6b291 to f7ed29a Compare October 23, 2019 17:37
@RobertCNelson RobertCNelson force-pushed the 4.14 branch 3 times, most recently from 334fb02 to 898fcce Compare January 28, 2020 16:48
@RobertCNelson RobertCNelson force-pushed the 4.14 branch 3 times, most recently from a4b9f96 to b532292 Compare March 19, 2020 20:31
@RobertCNelson RobertCNelson force-pushed the 4.14 branch 2 times, most recently from 28cf336 to fd90218 Compare April 30, 2020 20:18
RobertCNelson pushed a commit that referenced this pull request Sep 25, 2020
[ Upstream commit bad8e64 ]

On commit 6ac9311 ("blktrace: use existing disk debugfs directory")
merged on v4.12 Omar fixed the original blktrace code for request-based
drivers (multiqueue). This however left in place a possible crash, if you
happen to abuse blktrace while racing to remove / add a device.

We used to use asynchronous removal of the request_queue, and with that
the issue was easier to reproduce. Now that we have reverted to
synchronous removal of the request_queue, the issue is still possible to
reproduce, its however just a bit more difficult.

We essentially run two instances of break-blktrace which add/remove
a loop device, and setup a blktrace and just never tear the blktrace
down. We do this twice in parallel. This is easily reproduced with the
script run_0004.sh from break-blktrace [0].

We can end up with two types of panics each reflecting where we
race, one a failed blktrace setup:

[  252.426751] debugfs: Directory 'loop0' with parent 'block' already present!
[  252.432265] BUG: kernel NULL pointer dereference, address: 00000000000000a0
[  252.436592] #PF: supervisor write access in kernel mode
[  252.439822] #PF: error_code(0x0002) - not-present page
[  252.442967] PGD 0 P4D 0
[  252.444656] Oops: 0002 [#1] SMP NOPTI
[  252.446972] CPU: 10 PID: 1153 Comm: break-blktrace Tainted: G            E     5.7.0-rc2-next-20200420+ #164
[  252.452673] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014
[  252.456343] RIP: 0010:down_write+0x15/0x40
[  252.458146] Code: eb ca e8 ae 22 8d ff cc cc cc cc cc cc cc cc cc cc cc cc
               cc cc 0f 1f 44 00 00 55 48 89 fd e8 52 db ff ff 31 c0 ba 01 00
               00 00 <f0> 48 0f b1 55 00 75 0f 48 8b 04 25 c0 8b 01 00 48 89
               45 08 5d
[  252.463638] RSP: 0018:ffffa626415abcc8 EFLAGS: 00010246
[  252.464950] RAX: 0000000000000000 RBX: ffff958c25f0f5c0 RCX: ffffff8100000000
[  252.466727] RDX: 0000000000000001 RSI: ffffff8100000000 RDI: 00000000000000a0
[  252.468482] RBP: 00000000000000a0 R08: 0000000000000000 R09: 0000000000000001
[  252.470014] R10: 0000000000000000 R11: ffff958d1f9227ff R12: 0000000000000000
[  252.471473] R13: ffff958c25ea5380 R14: ffffffff8cce15f1 R15: 00000000000000a0
[  252.473346] FS:  00007f2e69dee540(0000) GS:ffff958c2fc80000(0000) knlGS:0000000000000000
[  252.475225] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  252.476267] CR2: 00000000000000a0 CR3: 0000000427d10004 CR4: 0000000000360ee0
[  252.477526] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  252.478776] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  252.479866] Call Trace:
[  252.480322]  simple_recursive_removal+0x4e/0x2e0
[  252.481078]  ? debugfs_remove+0x60/0x60
[  252.481725]  ? relay_destroy_buf+0x77/0xb0
[  252.482662]  debugfs_remove+0x40/0x60
[  252.483518]  blk_remove_buf_file_callback+0x5/0x10
[  252.484328]  relay_close_buf+0x2e/0x60
[  252.484930]  relay_open+0x1ce/0x2c0
[  252.485520]  do_blk_trace_setup+0x14f/0x2b0
[  252.486187]  __blk_trace_setup+0x54/0xb0
[  252.486803]  blk_trace_ioctl+0x90/0x140
[  252.487423]  ? do_sys_openat2+0x1ab/0x2d0
[  252.488053]  blkdev_ioctl+0x4d/0x260
[  252.488636]  block_ioctl+0x39/0x40
[  252.489139]  ksys_ioctl+0x87/0xc0
[  252.489675]  __x64_sys_ioctl+0x16/0x20
[  252.490380]  do_syscall_64+0x52/0x180
[  252.491032]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

And the other on the device removal:

[  128.528940] debugfs: Directory 'loop0' with parent 'block' already present!
[  128.615325] BUG: kernel NULL pointer dereference, address: 00000000000000a0
[  128.619537] #PF: supervisor write access in kernel mode
[  128.622700] #PF: error_code(0x0002) - not-present page
[  128.625842] PGD 0 P4D 0
[  128.627585] Oops: 0002 [#1] SMP NOPTI
[  128.629871] CPU: 12 PID: 544 Comm: break-blktrace Tainted: G            E     5.7.0-rc2-next-20200420+ #164
[  128.635595] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014
[  128.640471] RIP: 0010:down_write+0x15/0x40
[  128.643041] Code: eb ca e8 ae 22 8d ff cc cc cc cc cc cc cc cc cc cc cc cc
               cc cc 0f 1f 44 00 00 55 48 89 fd e8 52 db ff ff 31 c0 ba 01 00
               00 00 <f0> 48 0f b1 55 00 75 0f 65 48 8b 04 25 c0 8b 01 00 48 89
               45 08 5d
[  128.650180] RSP: 0018:ffffa9c3c05ebd78 EFLAGS: 00010246
[  128.651820] RAX: 0000000000000000 RBX: ffff8ae9a6370240 RCX: ffffff8100000000
[  128.653942] RDX: 0000000000000001 RSI: ffffff8100000000 RDI: 00000000000000a0
[  128.655720] RBP: 00000000000000a0 R08: 0000000000000002 R09: ffff8ae9afd2d3d0
[  128.657400] R10: 0000000000000056 R11: 0000000000000000 R12: 0000000000000000
[  128.659099] R13: 0000000000000000 R14: 0000000000000003 R15: 00000000000000a0
[  128.660500] FS:  00007febfd995540(0000) GS:ffff8ae9afd00000(0000) knlGS:0000000000000000
[  128.662204] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  128.663426] CR2: 00000000000000a0 CR3: 0000000420042003 CR4: 0000000000360ee0
[  128.664776] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  128.666022] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  128.667282] Call Trace:
[  128.667801]  simple_recursive_removal+0x4e/0x2e0
[  128.668663]  ? debugfs_remove+0x60/0x60
[  128.669368]  debugfs_remove+0x40/0x60
[  128.669985]  blk_trace_free+0xd/0x50
[  128.670593]  __blk_trace_remove+0x27/0x40
[  128.671274]  blk_trace_shutdown+0x30/0x40
[  128.671935]  blk_release_queue+0x95/0xf0
[  128.672589]  kobject_put+0xa5/0x1b0
[  128.673188]  disk_release+0xa2/0xc0
[  128.673786]  device_release+0x28/0x80
[  128.674376]  kobject_put+0xa5/0x1b0
[  128.674915]  loop_remove+0x39/0x50 [loop]
[  128.675511]  loop_control_ioctl+0x113/0x130 [loop]
[  128.676199]  ksys_ioctl+0x87/0xc0
[  128.676708]  __x64_sys_ioctl+0x16/0x20
[  128.677274]  do_syscall_64+0x52/0x180
[  128.677823]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

The common theme here is:

debugfs: Directory 'loop0' with parent 'block' already present

This crash happens because of how blktrace uses the debugfs directory
where it places its files. Upon init we always create the same directory
which would be needed by blktrace but we only do this for make_request
drivers (multiqueue) block drivers. When you race a removal of these
devices with a blktrace setup you end up in a situation where the
make_request recursive debugfs removal will sweep away the blktrace
files and then later blktrace will also try to remove individual
dentries which are already NULL. The inverse is also possible and hence
the two types of use after frees.

We don't create the block debugfs directory on init for these types of
block devices:

  * request-based block driver block devices
  * every possible partition
  * scsi-generic

And so, this race should in theory only be possible with make_request
drivers.

We can fix the UAF by simply re-using the debugfs directory for
make_request drivers (multiqueue) and only creating the ephemeral
directory for the other type of block devices. The new clarifications
on relying on the q->blk_trace_mutex *and* also checking for q->blk_trace
*prior* to processing a blktrace ensures the debugfs directories are
only created if no possible directory name clashes are possible.

This goes tested with:

  o nvme partitions
  o ISCSI with tgt, and blktracing against scsi-generic with:
    o block
    o tape
    o cdrom
    o media changer
  o blktests

This patch is part of the work which disputes the severity of
CVE-2019-19770 which shows this issue is not a core debugfs issue, but
a misuse of debugfs within blktace.

Fixes: 6ac9311 ("blktrace: use existing disk debugfs directory")
Reported-by: [email protected]
Signed-off-by: Luis Chamberlain <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Omar Sandoval <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Nicolai Stange <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: yu kuai <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
@RobertCNelson RobertCNelson force-pushed the 4.14 branch 2 times, most recently from cf8aa20 to 6ab314d Compare March 18, 2021 19:07
@RobertCNelson RobertCNelson force-pushed the 4.14 branch 2 times, most recently from bf0870f to 36b21ce Compare May 27, 2021 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.