Bug 2009928

Summary:	XkbChangeMap doesn't work, breaking certain keys in gnome onscreen keyboard
Product:	Red Hat Enterprise Linux 9	Reporter:	Ray Strode [halfline] <rstrode>
Component:	xorg-x11-server	Assignee:	Adam Jackson <ajax>
Status:	CLOSED ERRATA	QA Contact:	Michal Odehnal <modehnal>
Severity:	unspecified	Docs Contact:
Priority:	unspecified
Version:	CentOS Stream	CC:	bstinson, csoriano, jkoten, jwboyer, modehnal, tpelka
Target Milestone:	rc
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:	xorg-x11-server-1.20.11-4.el9	Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:
Clones:	2021614 (view as bug list)		Environment:
Last Closed:	2022-05-17 12:53:07 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:
Bug Depends On:
Bug Blocks:	2021614

Description Ray Strode [halfline] 2021-10-02 04:34:51 UTC

The GNOME onscreen keyboard has certain keys (like british pound, copyright symbol, etc) available that may not be part of the current keymap.

mutter deals with this situation, by adding them to the keymap at unused keycodes using the XkbChangeKMap call.

That call doesn't work right now.

After some digging, I believe I tracked the problem down to the fix for CVE-2020-14360 . It prevalidates what the length of the request should be and compares to the length reported in the request.

The XkbChangeKeymap call has a reported length much longer than the computed length.

The missing size is because this conditional is failing:

/**•
 * Check the length of the SetMap request•
 */•
static int•
_XkbSetMapCheckLength(xkbSetMapReq *req)•
{•
...
           if (req->flags & XkbSetMapResizeTypes) {•
                _add_check_len(keytype->nMapEntries•
                               * sz_xkbKTSetMapEntryWireDesc);•
...
                if (preserve) {•
                    _add_check_len(map_count * sz_xkbModsWireDesc);•
                }•

...
}•

Setting the flags check to be uncondtionally TRUE fixes things.

It doesn't look like libX11 ever sets XkbSetMapResizeTypes for the XkbChangeKeymap request (though it always sets it for XkbSetMap calls):

Bool•
XkbChangeMap(Display *dpy, XkbDescPtr xkb, XkbMapChangesPtr changes)•
{•
...
    req->flags = XkbSetMapRecomputeActions;•
...
}•

I need to dig a little more to figure out if the right fix is:

1) Make req->flags always set XkbSetMapResizeTypes
2) Make XkbChangeMap/_XkbWriteKeyTypes  write less on the wire since the flag isn't set
3) Change _XkbSetMapCheckLength to not check for the flag

Comment 1 Tomas Pelka 2021-10-02 19:58:46 UTC

Probably require fix for 8 and 7 too.

Comment 3 Ray Strode [halfline] 2021-10-04 18:12:50 UTC

Okay so I did a little more digging and mutter is doing extra work it doesn't need to that's leading to the failure. This (look for the removed lines below) side-steps the bug:

 static gboolean
 meta_keymap_x11_replace_keycode (MetaKeymapX11 *keymap_x11,
                                  KeyCode        keycode,
                                  KeySym         keysym)
 {
...
           int types[XkbNumKbdGroups] = { XkbOneLevelIndex };
           XkbChangeTypesOfKey (xkb, keycode, 1, XkbGroup1Mask, types, &changes);
           XkbKeySymEntry (xkb, keycode, 0, 0) = keysym;
...

-      changes.changed = XkbKeySymsMask | XkbKeyTypesMask;
-      changes.first_key_sym = keycode;
-      changes.num_key_syms = 1;
-      changes.first_type = 0;
-      changes.num_types = xkb->map->num_types;
       XkbChangeMap (xdisplay, xkb, &changes);
...
}

You see, mutter is telling the X server that it updated the key types data by putting XkbKeyTypesMask in the changed field.  But it didn't update the key types data.
All it does above is assign the "ONE_LEVEL" key type to the passed in key. "ONE_LEVEL" is a builtin key type that means the key shouldn't change its output when the shift key is held down.

From what I gather, XkbKeyTypesMask is for changing e.g. the number of shift levels a given key type has (e.g. you could copy ONE_LEVEL to a new key type MY_LEVELS and then give it 2 shift levels). 

So, anyway, mutter shouldn't be setting the mask saying the key type data changed and the key type data isn't getting touched.
This is extra clear, because it's passing for the "changed" types, 0 through map->num_types (all of them!). In other words, mutter is passing all the types unmodified to the X server for no particularly
good reason. 

Furthermore, XkbChangeTypesOfKey already preps the changed structure with the right data, because it's changing the key itself, so mutter doesn't actually need to mess around with the changed mask again itself at all.

Of course there's still an X server bug here. The X server only allows type data be sent in the request if the client says the number of types changed. That's wrong, which means 3 from comment 0 is the right fix. So I think we should fix the X server downstream, and simplify mutter upstream.

Comment 6 Ray Strode [halfline] 2021-10-26 17:10:46 UTC

https://gitlab.com/redhat/centos-stream/rpms/xorg-x11-server/-/merge_requests/3

Comment 13 errata-xmlrpc 2022-05-17 12:53:07 UTC

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (new packages: xorg-x11-server), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2022:2414