(release/25.0) Xi: bound SwapLongs of resolution values to the request length (backport to 25.0)

[metux]

Backport of commit 1fa7308 to release/25.0.

SProcXChangeDeviceControl() swapped "r->num_valuators" CARD32 in place after only checking that the request covered xDeviceResolutionCtl.

With "num_valuators" being a CARD8 from the wire, a short request for DEVICE_RESOLUTION could byte-swap up to 1020 bytes past the end of the request buffer.

Fix this issue by using the same extra-length check as ProcXChangeDeviceControl() before calling SwapLongs().

---

Verknüpfung

• Original auf master: (master) test/pyxtest: cover unbounded DEVICE_RESOLUTION SwapLongs #3772
• Geschwister-Backports: (release/25.2) RegionValidate: Fix double free of badreg->data on the error path #3794 ((master) RegionValidate: Fix double free of badreg->data on the error path #3779 / 25.2), (release/25.1) RegionValidate: Fix double free of badreg->data on the error path #3799 ((master) RegionValidate: Fix double free of badreg->data on the error path #3779 / 25.1), (release/25.0) RegionValidate: Fix double free of badreg->data on the error path #3805 ((master) RegionValidate: Fix double free of badreg->data on the error path #3779 / 25.0)

Release merges are manual, by the maintainer.

[metux]

🤖 Automated review — generated by Starfleet ship Enterprise (model: heavy-model) on behalf of @metux. Not a human review.

Review: pass — verified security backport, faithful to master

I checked the claims in the description against the actual release/25.0 source
rather than taking them on faith. All of them hold.

The vulnerability is real on 25.0

SProcXChangeDeviceControl() in Xi/chgdctl.c (pre-PR):

case DEVICE_RESOLUTION:
{
    xDeviceResolutionCtl *r = (xDeviceResolutionCtl *) &stuff[1];
    if (client->req_len - bytes_to_int32(sizeof(xChangeDeviceControlReq))
        < bytes_to_int32(sizeof(xDeviceResolutionCtl)))
        return BadLength;
    SwapLongs((CARD32 *) (r + 1), r->num_valuators);   /* unbounded */
    break;
}

xDeviceResolutionCtl.num_valuators is CARD8 (XIproto.h:1381), so a
byte-swapped client can claim up to 255 and drive SwapLongs 1020 bytes past
the end of the request buffer. The lower-bound check does not constrain it.

The fix matches ProcXChangeDeviceControl() exactly

ProcXChangeDeviceControl() already carried the correct check, and the PR uses
the same form:

if ((len < bytes_to_int32(sizeof(xDeviceResolutionCtl))) ||
    (len != bytes_to_int32(sizeof(xDeviceResolutionCtl)) + r->num_valuators)) {
    ret = BadLength;

which is precisely what this PR ports into the swapped-byte path. Correct layering:
the length invariant is enforced before the byte swap, the device-axis bound
(first_valuator + num_valuators > dev->valuator->numAxes) stays in Proc where
the device is known.

Two details that would have been real bugs, and are not

Both verified rather than assumed:

1. Reading num_valuators before SwapLongs is safe — only because it is
   CARD8. A single byte has no byte order, so the pre-swap value is already the
   final value. Had the field been CARD16/CARD32, this ordering would have been a
   bug in its own right.
2. sizeof(xDeviceResolutionCtl) + r->num_valuators is exact — the struct
   ends in explicit CARD8 pad1, pad2, so there is no implicit tail padding and
   (r + 1) points precisely at the resolution array. The arithmetic in the check
   matches the pointer arithmetic in the SwapLongs call.

On the +45 vs +6 discrepancy — the backport is complete

Worth recording, because the shape invites suspicion. On master the commit
1fa73081373b is +45/-0: it introduces the whole of
SProcXChangeDeviceControl() into Xext/xinput/chgdctl.c (the file was moved out
of Xi/ after 25.0 branched). The function already exists in 25.0's Xi/chgdctl.c,
so the same fix is +6/-2 there. I diffed the two: the fix logic is
character-for-character identical apart from master assigning r after the first
check instead of at the top of the block. Nothing in the master commit is missing
from the backport.

Also correctly carried over: the upstream provenance. The master commit records
Reported-by/Suggested-by (Adam Bedard), a Fixes: reference to
e24bd73e9d6f, three cherry picked from trailers, and a Part-of: link to the
upstream MR — the external form, pointing at xorg, not at us. This backport names
its origin commit in the body.

Rule 2 — backport assessment

This is the backport, and it is unambiguously security-relevant: a
byte-swapped client can byte-swap up to 1020 bytes past the request buffer, in
SProc, before any device validation. No authentication beyond a connected
X client. CWE-125 / CWE-787. Merge into release/25.0 is the maintainer's
call — I am reviewing and labelling only, not merging.

Rule 3 — driver ABI

No impact. No struct definition is touched, no _X_EXPORTed symbol is added or
removed (0 occurrences in the diff), and no driver-visible type changes. The
change is confined to a length check inside one request handler.