Harden IPU7 PSYS userptr pinning

[AFOliveira]

What

• Validate IPU7 PSYS userptr lengths and address arithmetic before calculating or allocating the page array.
• Use overflow-safe allocation, remove FOLL_FORCE, require a complete long-term pin, clean up partial pins, and publish attachment state only after SG creation succeeds.
• Clear page, page-count, and scatter-gather attachment state after release.
• Replace world-writable PSYS and runtime-directory permissions with video/seat-scoped access.
• Regenerate the driver patch with normal context and record that it has not yet been submitted upstream.
• Run the boundary regression, Linux checkpatch.pl, and an actual DKMS module build from check().
• Warn and continue package lifecycle work if immediate permission refresh fails; reboot then applies the packaged rules.

Why

The original driver narrows an attacker-controlled 64-bit length into signed page-count and allocation-size integers. A crafted request can therefore allocate space for one page pointer before pin_user_pages_fast() writes multiple pointers into that allocation. The package also exposed the PSYS device as mode 0666, making the ioctl reachable by every local account on affected hardware.

Review follow-up

• Every driver hunk now carries three lines of context; a deliberate anchor mutation makes application with --fuzz=0 fail.
• The package check compiles the patched driver and verifies intel-ipu7-psys.ko, rather than relying only on source greps and a duplicate arithmetic model.
• Scriptlet permission failures no longer skip daemon reload, service migration, enablement, or the reboot warning.
• The existing repeated-buffer pin/OOM possibility is an availability-hardening issue and remains outside this focused memory-corruption fix.

Verification

• The safe AddressSanitizer model reproduces the original 8-byte allocation followed by a page-pointer heap overflow.
• Both package patches apply to the pinned Intel source with --fuzz=0.
• Linux checkpatch.pl --no-tree --strict --no-signoff: 0 errors and 0 warnings.
• The exact package check() flow passes its ordinary, oversized, wrapping, and zero-length boundary cases.
• All six patched DKMS modules, including intel-ipu7-psys.ko, build successfully against Arch Linux 7.1.9 headers.
• PR linux-ptl: rebase to 7.2.2.arch1, carry all patches + Xe opportunistic compaction #271 does not publish its proposed linux-ptl 7.2.2 headers as an artifact, so compatibility with that unmerged kernel remains unverified.
• Source-array lengths and all non-VCS SHA-256 entries match.
• PKGBUILD and scriptlet Bash syntax checks pass.
• A mocked permission-refresh failure confirms package lifecycle operations continue.

The audit host has no IPU7 hardware, so the real ioctl was not executed against a live device/KASAN kernel. The finding is a source-confirmed kernel memory-safety defect, not a demonstrated end-to-end root exploit.

Deployment

The upgrade immediately attempts to remove world access from an existing device. If permission refresh fails, the scriptlet emits a reboot-before-use warning and continues the remaining lifecycle work. If intel_ipu7_psys is already loaded, reboot is required to replace the running module with the fixed build.

[omarchybot]

Reviewed. The bug you are closing is real and worse than the PR body claims, the fix is correct against the pinned source, and there are two packaging problems worth addressing before this lands.

What was actually checked. I read the patch against the real driver source at the commit this PKGBUILD pins (intel/ipu7-drivers @ a88b1909), applied it by hand, and verified every declared sha256sum in the PKGBUILD against the file in the tree — all 19 non-SKIP entries match, and the source/sums arrays are both 24 entries. I did not build this package: intel-ipu7-camera compiles a large HAL stack and the DKMS modules are compiled on the user's machine, so nothing here was proved by a build. Everything below is from reading source, not from running it.

What class of bug this closes. attach->len is u64 (ipu7-psys.h:153), while the original npages and array_size are signed 32-bit int (ipu-psys.c:54). array_size = npages * sizeof(struct page *) is computed as size_t and then narrowed back into int — so 0x20000001 pages need 0x100000008 bytes and array_size becomes 8. kvzalloc(8) gets room for one pointer, and pin_user_pages_fast() then writes struct page * values into it. That is a kernel heap out-of-bounds write.

The part worth being clear about: this does not require the attacker to own 2 TiB of memory. GUP writes pointers as it pins and returns a short count, and the old if (nr < npages) guard at ipu-psys.c:87 runs after those writes. So a caller submits a page-aligned GETBUF with a length of 0x20000001 * 4096 — that length is just a number — needs only two writable pages at the start followed by a hole, and pages[1] has already overrun the 8-byte allocation by the time the short-count check is reached. Combined with the old MODE="0666" on the PSYS node, that was reachable by any local account on an ordinary laptop. I had initially read this as needing an implausible amount of pinnable memory; codex worked out the short-count mechanism and it is right.

Which is also why, of the two halves of this PR, the udev and tmpfiles change is doing at least as much work as the C: 0666 → GROUP="video", MODE="0660", TAG+="uaccess" and /run/camera 0777 → 0770 root:video is what takes this off the "any local user" list.

The fix itself reads correct. Applied against the pinned source it produces the expected function: length validated before any arithmetic, check_add_overflow on start + len - 1, npages kept in size_t, kvmalloc_array instead of a pre-narrowed byte count, FOLL_FORCE dropped, a short pin rejected with the partial pin unwound, and attach->pages/npages/sgt published only after sg_alloc_table_from_pages succeeds. long nr losing its = 0 initialiser is safe — every path to unpin_pages: is after the pin_user_pages_fast() assignment, and the allocation-failure path jumps past it to free_sgt. MAX_RW_COUNT is INT_MAX & PAGE_MASK from <linux/fs.h>, which this file includes directly at line 9.

One thing I suspected and could not make stick, recorded so nobody re-treads it: ipu7_psys_put_userpages() (ipu-psys.c:118) still leaves attach->pages and attach->npages set after kvfree, and ipu7_dma_buf_vmap()/vunmap() test exactly those fields. Codex traced the lifecycle and found no reachable use-after-free — detach frees the private struct immediately after put_userpages(), vunmap happens before detach, and attach->sgt = NULL short-circuits a repeat call. Clearing all three would still be cheap hygiene while you are in this function.

Two packaging problems:

1. The patch is diff -U0, and three of its hunks are pure insertions — @@ -15,0 +16 @@, @@ -98,0 +101,2 @@ and @@ -579,0 +581,5 @@ (patch lines 18, 82, 94). A hunk with no context and no removed lines gives patch(1) nothing to verify: it inserts at a line number and reports success either way. Today it is fine — 0004 only touches old line 1539, which is below 0005's highest anchor at 579, so nothing shifts — but the +581 hunk is the exposed one, with no anchoring hunk between old lines 108 and 579. If the pinned commit moves or 0004 grows a hunk higher in the file, that five-line splice lands somewhere else and still "applies". Regenerating this with normal context (-U3) costs nothing and removes the whole class.

   Related: the @@ -579,0 +581,5 @@ hunk opens a brace it does not close, and relies on the original lines 580-581 to close it. It is correct against the pinned source — I worked it through and so did codex — but it is very hard to review, and that is a bad property for a security patch.

2. check() cannot catch that. PKGBUILD:162-175 greps for two strings, asserts FOLL_FORCE is absent, and compiles check-userptr-range.c — which re-implements the arithmetic rather than testing the driver. It would pass a misplaced #include <linux/overflow.h> or a attach->pages = pages; spliced into the wrong block, because the grepped strings would still be present. A syntax-damaging splice would first show up when DKMS builds on a user's machine, leaving them without a camera. The helper is a fine unit test of the formula; it just is not evidence about the shipped module.

intel-ipu7-camera.install. _apply_runtime_permissions || return 1 in post_install/post_upgrade does not do what it looks like it does. A non-zero return from a pacman scriptlet does not roll back or fail the transaction — libalpm runs scriptlets after committing the package and only logs the failure. So if systemd-tmpfiles --create or the udev reload fails, the package is installed but everything after that line is skipped: daemon-reload, the camera-init/v4l2-relayd service migration, systemctl enable intel-ipu7-camera.service, the reboot warning, and the per-user config. That is a worse state than the one it is guarding against, and it buys no fail-closed guarantee. Warning and continuing would be safer. (The container/chroot case is already handled — line 28 skips udevadm unless /run/udev/control exists.)

Permissions check out. 70- is early enough: it runs before systemd's 73-seat-late.rules, so TAG+="uaccess" is set when logind assigns the seat ACL. Nothing in the package needs the old permissions — camera-init.service, intel-ipu7-camera.service and the upstream v4l2-relayd@.service all run as root with no User=/Group=, the sleep hook runs in system context, and no other consumer of /run/camera assumes 0777.

Not addressed, and pre-existing: there is still no aggregate pin limit or locked-memory accounting. Each MAPBUF retains another long-term-pinned range in fh->bufmap, so a video-group or active-seat user can pin repeatedly until OOM. Availability rather than corruption, and older than this PR, but it sits squarely under "harden userptr pinning" if you want it in scope.

On PR #271 (linux-ptl → 7.2.2.arch1): no file overlap — #271 touches only pkgbuilds/linux-ptl/, and its "carries all patches" refers to linux-ptl's own series, not this one. The interaction is that this patch is verified against 7.1.9 only. These are DKMS modules, so a 7.2.2 incompatibility surfaces as a build failure on users' machines at kernel upgrade rather than in CI. I could not test it — linux-ptl in the repo is still 7.1.8, so 7.2.2 headers were not available.

An upstream backport should name its commit, and this one does not. The patch header (From 0000000…, Subject: [PATCH] media: intel/ipu7: harden PSYS userptr pinning) is authored here rather than cherry-picked, with no Upstream-Status or lore link. If this has been sent to linux-media, say so in the file; if it has not, saying that is just as useful, because it tells the next person whether to expect it to disappear at the next rebase.

Second opinion: codex at xhigh reviewed this independently. It contributed the short-count exploitation mechanism above, which changed my conclusion about severity; it disproved the use-after-free I suspected in put_userpages(); and it established the pacman scriptlet semantics and the service-unit ownership in the permissions section. Where we agree — the zero-context insertion hunks, the check() coverage gap, the nr initialisation being safe — its independence is not currently guaranteed, so read that as two readings rather than two proofs.

Nothing was pushed to this branch: the patch format and the install-script behaviour are both your calls to make rather than mine to make for you. Waiting on the author.

[omarchybot]

Re-reviewed at 7677d8f. All four things from the last pass are fixed, and this time I proved each one by running it rather than by reading it. Two things are worth your attention before this lands, one of which is not your bug.

Everything below ran on a disposable Omarchy worker VM (kernel 7.1.9-arch1-2, linux-headers installed), never on the machine doing the review.

The four previous findings, each checked by execution.

1. The diff -U0 pure-insertion hunks. Fixed. The patch now carries three lines of context, and its index fee9ee8..c6c4b52 header is truthful: applying 0004 to the pinned intel/ipu7-drivers @ a88b1909 produces blob fee9ee82465bb07eb9212f0df702448ec1c1cfc9, and applying 0005 on top produces c6c4b52a8f3ee8f307e5f5c06595e75177a5d391. Both hashes match the patch header exactly. I also mutated a context line in the pinned source and confirmed 0005 then fails at --fuzz=0 — the anchoring is real.
2. The scriptlet's return 1. Fixed, and I tested the semantics rather than asserting them: I built a throwaway package on the worker whose post_install reproduces the new shape — a failing permission step, then a trailing if [ -S ... ] that is false — and installed it. Both markers after _apply_runtime_permissions printed and pacman reported no scriptlet failure. So the daemon reload, the camera-init/v4l2-relayd migration and systemctl enable do now run. The non-zero return _apply_runtime_permissions gives when /run/udev/control is absent is harmless for the same reason.
3. check() could not catch a misplaced splice. Fixed, and this is the change I am most glad about. I mutated the patched tree so the five-line getbuf block lands at file scope, then ran the old check()'s three greps against it: all three still passed. The new check() fails it — ipu-psys.c:46:9: error: expected identifier or '(' before 'if', make exits 2. I then ran your check() body verbatim under errexit in a simulated srcdir: exit 0, checkpatch.pl --no-tree --strict --no-signoff reports 0 errors, 0 warnings, 0 checks, the boundary helper passes, all six modules build, intel-ipu7-psys.ko is 1431400 bytes, the ipu7-drivers-check copy is cleaned up, and — the part that matters for DKMS — zero .o/.ko artefacts are left in ipu7-drivers/, which is what package() ships into /usr/src.
4. No Upstream-Status. Fixed.

One thing pushed: 6e77366. Your PR body says a deliberate anchor mutation makes application with --fuzz=0 fail. True of the patch — but prepare() called patch -Np1 with GNU patch's default fuzz of 2, so the build never asked for that guarantee. Against the same mutated source, the default invocation reported Hunk #1 succeeded at 13 with fuzz 1 and applied. The commit adds --fuzz=0 to both patch calls. Re-verified on the worker afterwards: both patches apply at fuzz zero against the pinned commit, checkpatch stays clean, the helper passes, the module still builds.

A second bug this patch closes that neither of us named. Pre-patch, nr is int and the cleanup is if (nr) unpin_user_pages(pages, nr). unpin_user_pages() takes an unsigned long, so a negative return from pin_user_pages_fast() — which happens when zero pages were pinned, including the FOLL_LONGTERM migration failures in check_and_migrate_movable_pages() that the old vma_lookup guard does not cover — sign-extends into 18446744073709551602. I loaded a module on the worker that makes exactly that call with nr = -EFAULT: the kernel answers IS_ERR_VALUE(npages) / WARNING: mm/gup.c:412 at unpin_user_pages+0x108/0x110. So it is a guarded WARN rather than corruption — but it is a splat a local caller can provoke, and a panic on a panic_on_warn kernel. Your if (nr > 0) closes it.

The one that is not your bug, and is the reason to be careful about FOLL_FORCE. ipu7_psys_mapbuf_locked() leaves kbuf->db_attach invalid on both of its error paths. At ipu-psys.c:734 a failed dma_buf_attach() stores an ERR_PTR and attach_fail: (line 792) never clears it; for a userptr buffer line 794 deliberately does not free kbuf, so it survives, owned by the exported fd. Closing that fd runs ipu7_dma_buf_release(), which at line 226 tests if (kbuf->db_attach) — true for an ERR_PTR — and dereferences it at line 227. The kbuf_map_fail: path is the same shape with a freed pointer instead of an ERR_PTR, via the dma_buf_detach() at line 790.

This is pre-existing and shipping today: pre-patch, a GETBUF with an unmapped page-aligned userptr fails vma_lookup() with -EFAULT and reaches the identical path. Your patch neither introduces nor fixes it, and I did not push a fix — clearing kbuf->db_attach on both paths is a second hunk in a function you deliberately left alone, it would mean hand-editing this patch's index hashes and its checksum, and it is a change I cannot test on the hardware. It should be your call, and arguably its own patch.

But it changes how the FOLL_FORCE removal reads. Dropping FOLL_FORCE means a MAP_PRIVATE buffer that was created writable and later mprotect(PROT_READ)'d — it keeps VM_MAYWRITE but loses VM_WRITE — now fails to pin where it used to succeed. On its own that is just MAPBUF returning -EFAULT. Combined with the bug above, the next close() of that fd oopses. I think the risk is low, because the driver requests FOLL_WRITE for every buffer regardless of direction, so anything relying on FOLL_FORCE was already getting a COW copy the hardware's writes would never reach userspace through — i.e. already broken. But that is an argument, not a test.

What could not be tested at all: there is no IPU7 hardware here. No ioctl was issued against a live PSYS device, no KASAN kernel ran, and the camera was never brought up end to end. The overflow is a source-confirmed defect and the fix reads correct; it is not a demonstrated exploit, and the FOLL_FORCE change is not proven harmless on a real camera. Everything above is compilation, patch application, scriptlet behaviour and udev/tmpfiles mechanics.

Permissions re-checked, and this time run. There is exactly one PSYS rules file now — 90-ipu7-psys.rules (MODE="0666") is deleted and 70-ipu7-psys.rules added — so there is nothing to contradict; 71-ipu7-hide-isys.rules matches video4linux ISYS capture nodes, not ipu7-psys0. udevadm verify passes the new rule. 70- sorts before systemd's 73-seat-late.rules, whose line 16 is TAG=="uaccess|xaccess-*", ENV{MAJOR}!="", RUN{builtin}+="uaccess", and psys->dev.devt is set at ipu-psys.c:1423, so MAJOR is populated and the seat ACL lands. systemd-tmpfiles --create demoted a pre-existing 0777 root:root /run/camera to 0770 root:video on the worker and was idempotent on a second run, so the upgrade path actually remediates rather than only affecting fresh installs.

On whether this locks the camera out: v4l2-relayd@.service sets CapabilityBoundingSet= empty, which drops CAP_DAC_OVERRIDE, so "it runs as root" is not by itself the answer. It still works, because the node stays owned by root and DAC grants the owner bits without any capability. I checked that rather than reasoned it: a systemd-run unit with CapabilityBoundingSet= empty wrote to a 0660 root:video file successfully. No unit in the package sets User=, Group= or DynamicUser=.

Also verified: all 24 source entries have 24 checksums, all 19 non-SKIP hashes match the files in the tree, pkgrel is bumped to 2 so this reaches users, and makepkg's run_function_safe sets shopt -o -s errexit, so the bare greps and the test -s in check() really do abort the build. !npages and npages > INT_MAX are unreachable given MAX_RW_COUNT caps the count at 524288, and the later narrowings to pin_user_pages_fast()'s int and sg_alloc_table_from_pages()'s unsigned int are safe for the same reason.

Still open from last time: no aggregate pin limit or locked-memory accounting. Each MAPBUF retains another long-term pin in fh->bufmap, so a video-group or active-seat user can pin until OOM. You have said that is availability hardening and out of scope for this fix, which is a reasonable line to draw.

Second opinion: codex at xhigh reviewed this independently. The kbuf->db_attach finding is entirely its own — I had traced the vmap/vunmap and put_userpages lifecycles and missed the mapbuf error paths completely — as is the mprotect(PROT_READ) mechanism for the FOLL_FORCE interaction. I verified both against the source and the line numbers hold; the one thing I did not accept is its framing of that as a new trigger, since the unmapped-userptr path reaches the same oops before this patch. It agreed on the arithmetic, the partial-pin cleanup, the udev ordering, the scriptlet and the checksums, and on those its independence is not currently guaranteed, so read them as two readings rather than two proofs. It could not run checkpatch or the module build — no linux-headers where it ran — so those results are mine from the worker.

Waiting on you for whether the kbuf->db_attach cleanup belongs in this PR or its own, and on the maintainer for the merge. This is still a draft PR.