[Git][xorg-team/xserver/xorg-server][upstream-unstable] 9 commits: dix: Fix use after free in input device shutdown

Julien Cristau pushed to branch upstream-unstable at X Strike Force / xserver / xorg-server

Commits:

8b75ec34

by Povilas Kanapickas at 2024-01-22T15:14:21+10:00

dix: Fix use after free in input device shutdown

This fixes access to freed heap memory via dev->master. E.g. when
running BarrierNotify.ReceivesNotifyEvents/7 test from
xorg-integration-tests:

==24736==ERROR: AddressSanitizer: heap-use-after-free on address
0x619000065020 at pc 0x55c450e2b9cf bp 0x7fffc532fd20 sp 0x7fffc532fd10
READ of size 4 at 0x619000065020 thread T0
    #0 0x55c450e2b9ce in GetMaster ../../../dix/devices.c:2722
    #1 0x55c450e9d035 in IsFloating ../../../dix/events.c:346
    #2 0x55c4513209c6 in GetDeviceUse ../../../Xi/xiquerydevice.c:525
../../../Xi/xichangehierarchy.c:95
    #4 0x55c450e3455c in RemoveDevice ../../../dix/devices.c:1204
../../../hw/xfree86/common/xf86Xinput.c:1142
    #6 0x55c450e17b04 in CloseDeviceList ../../../dix/devices.c:1038
    #7 0x55c450e1de85 in CloseDownDevices ../../../dix/devices.c:1068
    #8 0x55c450e837ef in dix_main ../../../dix/main.c:302
    #9 0x55c4517a8d93 in main ../../../dix/stubmain.c:34
(/lib/x86_64-linux-gnu/libc.so.6+0x28564)
    #11 0x55c450d0113d in _start (/usr/lib/xorg/Xorg+0x117713d)

0x619000065020 is located 160 bytes inside of 912-byte region
[0x619000064f80,0x619000065310)
freed by thread T0 here:
(/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10d7cf)
    #1 0x55c450e19f1c in CloseDevice ../../../dix/devices.c:1014
    #2 0x55c450e343a4 in RemoveDevice ../../../dix/devices.c:1186
../../../hw/xfree86/common/xf86Xinput.c:1142
    #4 0x55c450e17b04 in CloseDeviceList ../../../dix/devices.c:1038
    #5 0x55c450e1de85 in CloseDownDevices ../../../dix/devices.c:1068
    #6 0x55c450e837ef in dix_main ../../../dix/main.c:302
    #7 0x55c4517a8d93 in main ../../../dix/stubmain.c:34
(/lib/x86_64-linux-gnu/libc.so.6+0x28564)

previously allocated by thread T0 here:
(/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10ddc6)
    #1 0x55c450e1c57b in AddInputDevice ../../../dix/devices.c:259
    #2 0x55c450e34840 in AllocDevicePair ../../../dix/devices.c:2755
    #3 0x55c45130318f in add_master ../../../Xi/xichangehierarchy.c:152
../../../Xi/xichangehierarchy.c:465
    #5 0x55c4512cb9f5 in ProcIDispatch ../../../Xi/extinit.c:390
    #6 0x55c450e6a92b in Dispatch ../../../dix/dispatch.c:551
    #7 0x55c450e834b7 in dix_main ../../../dix/main.c:272
    #8 0x55c4517a8d93 in main ../../../dix/stubmain.c:34
(/lib/x86_64-linux-gnu/libc.so.6+0x28564)

The problem is caused by dev->master being not reset when disabling the
device, which then causes dangling pointer when the master device itself
is being deleted when exiting whole server.

Note that RecalculateMasterButtons() requires dev->master to be still
valid, so we can reset it only at the end of function.

Signed-off-by: Povilas Kanapickas <povilas@radix.lt>
(cherry picked from commit 1801fe0ac3926882d47d7e1ad6c0518a2cdffd41)

f653d9a0

by Yusuf Khan at 2024-02-19T10:12:03+00:00

hw/xfree86: fix NULL pointer refrence to mode name

Potentially, the pointer to the mode name could be unset, this can
occur with the xf86-video-nv DDX, in that case there isnt much we can do
except check if the next mode is any better.

Signed-off-by: Yusuf Khan <yusisamerican@gmail.com>
---
(cherry picked from db3aa4e03b180244e8b4b02272c49f1e0c48b463)

Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1309>

8a46a463

by Matthieu Herrb at 2024-02-23T00:01:10+00:00

Initialize Mode->name in xf86CVTMode()

This was overlooked when converting the function to use libxcvt.
Bring back name initialization from old code.

This was causing a segfault in xf86LookupMode() if modes where
name is NULL are present the modePool list.

Signed-off-by: Matthieu Herrb <matthieu@herrb.eu>
---
(cherry picked from ed11c4d443ad2e82512df64358d38008e0ee7693)

Reported-by: "Sergiy" <Black_N@ukr.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1319>

5d7272f0

by Peter Hutterer at 2024-03-23T14:42:15-07:00

Allow disabling byte-swapped clients

The X server swapping code is a huge attack surface, much of this code
is untested and prone to security issues. The use-case of byte-swapped
clients is very niche, so allow users to disable this if they don't
need it, using either a config option or commandline flag.

For Xorg, this adds the ServerFlag "AllowByteSwappedClients" "off".
For all DDX, this adds the commandline options +byteswappedclients and
-byteswappedclients to enable or disable, respectively.

Fixes #1201
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
(cherry picked from commit 412777664a20dd3561b936c02c96571a756fe9b2)
(cherry picked from commit af5cd5acc9012e527ee869f8e98bf6c2e9a02ca4)
Backport to server-21.1-branch modified to keep byte-swapping enabled
by default but easy to disable by users or admins (or even by distros
shipping an xorg.conf.d fragment in their packages).

Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1440>

5ca3a951

by Alan Coopersmith at 2024-03-27T19:39:19+00:00

Xext: SProcSyncCreateFence needs to swap drawable id too

Otherwise it causes the server to return BadDrawable giving a
byte-swapped resource id instead of the real id the client sent.

Reported-by: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=69762
Fixes: 397dfd9f8 ("Create/Destroy/Trigger/Reset/Query Fence Sync objs")
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
---
(cherry picked from commit e6573baa7d99a77f44229b9a96a41bbda57e2387)

Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1438>

8a7cd0e3

by Alan Coopersmith at 2024-04-03T19:35:30+03:00

Xi: ProcXIGetSelectedEvents needs to use unswapped length to send reply

CVE-2024-31080

Reported-by: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=69762
Fixes: 53e821ab4 ("Xi: add request processing for XIGetSelectedEvents.")
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1463>
(cherry picked from commit 96798fc1967491c80a4d0c8d9e0a80586cb2152b)

cea92ca7

by Alan Coopersmith at 2024-04-03T19:35:39+03:00

Xi: ProcXIPassiveGrabDevice needs to use unswapped length to send reply

CVE-2024-31081

Fixes: d220d6907 ("Xi: add GrabButton and GrabKeysym code.")
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1463>
(cherry picked from commit 3e77295f888c67fc7645db5d0c00926a29ffecee)

0e34d8eb

by Alan Coopersmith at 2024-04-03T19:35:46+03:00

Xquartz: ProcAppleDRICreatePixmap needs to use unswapped length to send reply

CVE-2024-31082

Fixes: 14205ade0 ("XQuartz: appledri: Fix byte swapping in replies")
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1463>
(cherry picked from commit 6c684d035c06fd41c727f0ef0744517580864cef)

11731564

by Peter Hutterer at 2024-04-03T19:37:08+03:00

render: fix refcounting of glyphs during ProcRenderAddGlyphs

Previously, AllocateGlyph would return a new glyph with refcount=0 and a
re-used glyph would end up not changing the refcount at all. The
resulting glyph_new array would thus have multiple entries pointing to
the same non-refcounted glyphs.

AddGlyph may free a glyph, resulting in a UAF when the same glyph
pointer is then later used.

Fix this by returning a refcount of 1 for a new glyph and always
incrementing the refcount for a re-used glyph, followed by dropping that
refcount back down again when we're done with it.

CVE-2024-31083, ZDI-CAN-22880

This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative

Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1463>
(cherry picked from commit bdca6c3d1f5057eeb31609b1280fc93237b00c77)

16 changed files:

Xext/sync.c
Xi/xipassivegrab.c
Xi/xiselectev.c
dix/devices.c
dix/dispatch.c
hw/xfree86/common/xf86Config.c
hw/xfree86/common/xf86Mode.c
hw/xfree86/man/xorg.conf.man
hw/xfree86/modes/xf86Modes.c
hw/xquartz/xpr/appledri.c
include/opaque.h
man/Xserver.man
os/utils.c
render/glyph.c
render/glyphstr.h
render/render.c

Changes:

Xext/sync.c

@@ -2318,6 +2318,7 @@ SProcSyncCreateFence(ClientPtr client)
      REQUEST(xSyncCreateFenceReq);
      swaps(&stuff->length);
      REQUEST_SIZE_MATCH(xSyncCreateFenceReq);
 +    swapl(&stuff->d);
      swapl(&stuff->fid);
      return ProcSyncCreateFence(client);

Xi/xipassivegrab.c

@@ -93,6 +93,7 @@ ProcXIPassiveGrabDevice(ClientPtr client)
      GrabParameters param;
      void *tmp;
      int mask_len;
 +    uint32_t length;
      REQUEST(xXIPassiveGrabDeviceReq);
      REQUEST_FIXED_SIZE(xXIPassiveGrabDeviceReq,
@@ -247,9 +248,11 @@ ProcXIPassiveGrabDevice(ClientPtr client)
+         }
+     }
 +    /* save the value before SRepXIPassiveGrabDevice swaps it */
 +    length = rep.length;
      WriteReplyToClient(client, sizeof(rep), &rep);
      if (rep.num_modifiers)
 -        WriteToClient(client, rep.length * 4, modifiers_failed);
 +        WriteToClient(client, length * 4, modifiers_failed);
   out:
      free(modifiers_failed);

Xi/xiselectev.c

@@ -349,6 +349,7 @@ ProcXIGetSelectedEvents(ClientPtr client)
      InputClientsPtr others = NULL;
      xXIEventMask *evmask = NULL;
      DeviceIntPtr dev;
 +    uint32_t length;
      REQUEST(xXIGetSelectedEventsReq);
      REQUEST_SIZE_MATCH(xXIGetSelectedEventsReq);
@@ -418,10 +419,12 @@ ProcXIGetSelectedEvents(ClientPtr client)
+         }
+     }
 +    /* save the value before SRepXIGetSelectedEvents swaps it */
 +    length = reply.length;
      WriteReplyToClient(client, sizeof(xXIGetSelectedEventsReply), &reply);
      if (reply.num_masks)
 -        WriteToClient(client, reply.length * 4, buffer);
 +        WriteToClient(client, length * 4, buffer);
      free(buffer);
      return Success;

dix/devices.c


     }
 
     RecalculateMasterButtons(dev);
+    dev->master = NULL;
 
     return TRUE;
 }

dix/dispatch.c

@@ -3780,9 +3780,11 @@ ProcEstablishConnection(ClientPtr client)
      auth_proto = (char *) prefix + sz_xConnClientPrefix;
      auth_string = auth_proto + pad_to_int32(prefix->nbytesAuthProto);
 -    if ((client->req_len << 2) != sz_xReq + sz_xConnClientPrefix +
 -	pad_to_int32(prefix->nbytesAuthProto) +
 -	pad_to_int32(prefix->nbytesAuthString))
 +    if (client->swapped && !AllowByteSwappedClients) {
 +        reason = "Prohibited client endianess, see the Xserver man page ";
 +    } else if ((client->req_len << 2) != sz_xReq + sz_xConnClientPrefix +
 +               pad_to_int32(prefix->nbytesAuthProto) +
 +               pad_to_int32(prefix->nbytesAuthString))
          reason = "Bad length";
      else if ((prefix->majorVersion != X_PROTOCOL) ||
          (prefix->minorVersion != X_PROTOCOL_REVISION))

hw/xfree86/common/xf86Config.c

@@ -646,6 +646,7 @@ typedef enum {
      FLAG_MAX_CLIENTS,
      FLAG_IGLX,
      FLAG_DEBUG,
 +    FLAG_ALLOW_BYTE_SWAPPED_CLIENTS,
  } FlagValues;
  /**
@@ -705,6 +706,8 @@ static OptionInfoRec FlagOptions[] = {
       {0}, FALSE},
      {FLAG_DEBUG, "Debug", OPTV_STRING,
       {0}, FALSE},
 +    {FLAG_ALLOW_BYTE_SWAPPED_CLIENTS, "AllowByteSwappedClients", OPTV_BOOLEAN,
 +     {0}, FALSE},
      {-1, NULL, OPTV_NONE,
       {0}, FALSE},
  };
@@ -746,6 +749,14 @@ configServerFlags(XF86ConfFlagsPtr flagsconf, XF86OptionPtr layoutopts)
          xf86Msg(X_CONFIG, "Ignoring ABI Version\n");
+     }
 +    xf86GetOptValBool(FlagOptions, FLAG_ALLOW_BYTE_SWAPPED_CLIENTS, &AllowByteSwappedClients);
 +    if (AllowByteSwappedClients) {
 +        xf86Msg(X_CONFIG, "Allowing byte-swapped clients\n");
 +    }
 +    else {
 +        xf86Msg(X_CONFIG, "Prohibiting byte-swapped clients\n");
 +    }
++
      if (xf86IsOptionSet(FlagOptions, FLAG_AUTO_ADD_DEVICES)) {
          xf86GetOptValBool(FlagOptions, FLAG_AUTO_ADD_DEVICES,
                            &xf86Info.autoAddDevices);

hw/xfree86/common/xf86Mode.c

@@ -507,6 +507,8 @@ xf86LookupMode(ScrnInfoPtr scrp, DisplayModePtr modep,
              /* scan through the modes in the sort order above */
              if ((p->type & type) != type)
                  continue;
 +            if (p->name == NULL)
 +                continue;
              if (strcmp(p->name, modep->name) == 0) {

hw/xfree86/man/xorg.conf.man

@@ -677,6 +677,9 @@ Possible values are
  or
  .BR sync .
  Unset by default.
 +.TP 7
 +.BI "Option \*qAllowByteSwappedClients\*q  \*q" boolean \*q
 +Allow clients with a different byte-order than the server. Enabled by default.
  .SH "MODULE SECTION"
  The
  .B Module

hw/xfree86/modes/xf86Modes.c

@@ -803,10 +803,14 @@ xf86CVTMode(int HDisplay, int VDisplay, float VRefresh, Bool Reduced,
+ {
      struct libxcvt_mode_info *libxcvt_mode_info;
      DisplayModeRec *Mode = xnfcalloc(1, sizeof(DisplayModeRec));
 +    char *tmp;
      libxcvt_mode_info =
          libxcvt_gen_mode_info(HDisplay, VDisplay, VRefresh, Reduced, Interlaced);
 +    XNFasprintf(&tmp, "%dx%d", HDisplay, VDisplay);
 +    Mode->name = tmp;
++
      Mode->VDisplay   = libxcvt_mode_info->vdisplay;
      Mode->HDisplay   = libxcvt_mode_info->hdisplay;
      Mode->Clock      = libxcvt_mode_info->dot_clock;

hw/xquartz/xpr/appledri.c

@@ -272,6 +272,7 @@ ProcAppleDRICreatePixmap(ClientPtr client)
      xAppleDRICreatePixmapReply rep;
      int width, height, pitch, bpp;
      void *ptr;
 +    CARD32 stringLength;
      REQUEST_SIZE_MATCH(xAppleDRICreatePixmapReq);
@@ -307,6 +308,7 @@ ProcAppleDRICreatePixmap(ClientPtr client)
      if (sizeof(rep) != sz_xAppleDRICreatePixmapReply)
          ErrorF("error sizeof(rep) is %zu\n", sizeof(rep));
 +    stringLength = rep.stringLength;  /* save unswapped value */
      if (client->swapped) {
          swaps(&rep.sequenceNumber);
          swapl(&rep.length);
@@ -319,7 +321,7 @@ ProcAppleDRICreatePixmap(ClientPtr client)
+     }
      WriteToClient(client, sizeof(rep), &rep);
 -    WriteToClient(client, rep.stringLength, path);
 +    WriteToClient(client, stringLength, path);
      return Success;
+ }

include/opaque.h

@@ -74,4 +74,6 @@ extern _X_EXPORT Bool bgNoneRoot;
  extern _X_EXPORT Bool CoreDump;
  extern _X_EXPORT Bool NoListenAll;
 +extern _X_EXPORT Bool AllowByteSwappedClients;
++
  #endif                          /* OPAQUE_H */

man/Xserver.man

@@ -114,6 +114,13 @@ pattern.   This is the default unless -retro or -wr is specified.
  .B \-bs
  disables backing store support on all screens.
  .TP 8
 +.B \+byteswappedclients
 +Allow connections from clients with an endianess different to that of the server.
 +This is the default unless \fB\-byteswappedclients\fP is specified.
 +.TP 8
 +.B \-byteswappedclients
 +Prohibit connections from clients with an endianess different to that of the server.
 +.TP 8
  .B \-c
  turns off key-click.
  .TP 8

os/utils.c

@@ -189,6 +189,8 @@ Bool CoreDump;
  Bool enableIndirectGLX = FALSE;
 +Bool AllowByteSwappedClients = TRUE;
++
  #ifdef PANORAMIX
  Bool PanoramiXExtensionDisabledHack = FALSE;
  #endif
@@ -523,6 +525,8 @@ UseMsg(void)
      ErrorF("-br                    create root window with black background\n");
      ErrorF("+bs                    enable any backing store support\n");
      ErrorF("-bs                    disable any backing store support\n");
 +    ErrorF("+byteswappedclients    Allow clients with endianess different to that of the server\n");
 +    ErrorF("-byteswappedclients    Prohibit clients with endianess different to that of the server\n");
      ErrorF("-c                     turns off key-click\n");
      ErrorF("c #                    key-click volume (0-100)\n");
      ErrorF("-cc int                default color visual class\n");
@@ -719,6 +723,11 @@ ProcessCommandLine(int argc, char *argv[])
              else
                  UseMsg();
+         }
 +        else if (strcmp(argv[i], "-byteswappedclients") == 0) {
 +            AllowByteSwappedClients = FALSE;
 +        } else if (strcmp(argv[i], "+byteswappedclients") == 0) {
 +            AllowByteSwappedClients = TRUE;
 +        }
          else if (strcmp(argv[i], "-br") == 0);  /* default */
          else if (strcmp(argv[i], "+bs") == 0)
              enableBackingStore = TRUE;

render/glyph.c

@@ -245,10 +245,11 @@ FreeGlyphPicture(GlyphPtr glyph)
+     }
+ }
 -static void
 +void
  FreeGlyph(GlyphPtr glyph, int format)
+ {
      CheckDuplicates(&globalGlyphs[format], "FreeGlyph");
 +    BUG_RETURN(glyph->refcnt == 0);
      if (--glyph->refcnt == 0) {
          GlyphRefPtr gr;
          int i;
@@ -354,7 +355,7 @@ AllocateGlyph(xGlyphInfo * gi, int fdepth)
      glyph = (GlyphPtr) malloc(size);
      if (!glyph)
          return 0;
 -    glyph->refcnt = 0;
 +    glyph->refcnt = 1;
      glyph->size = size + sizeof(xGlyphInfo);
      glyph->info = *gi;
      dixInitPrivates(glyph, (char *) glyph + head_size, PRIVATE_GLYPH);

render/glyphstr.h

@@ -109,6 +109,8 @@ extern GlyphPtr FindGlyph(GlyphSetPtr glyphSet, Glyph id);
  extern GlyphPtr AllocateGlyph(xGlyphInfo * gi, int format);
 +extern void FreeGlyph(GlyphPtr glyph, int format);
++
  extern Bool
   ResizeGlyphSet(GlyphSetPtr glyphSet, CARD32 change);

render/render.c

@@ -1076,6 +1076,7 @@ ProcRenderAddGlyphs(ClientPtr client)
          if (glyph_new->glyph && glyph_new->glyph != DeletedGlyph) {
              glyph_new->found = TRUE;
 +            ++glyph_new->glyph->refcnt;
+         }
          else {
              GlyphPtr glyph;
@@ -1168,8 +1169,10 @@ ProcRenderAddGlyphs(ClientPtr client)
          err = BadAlloc;
          goto bail;
+     }
 -    for (i = 0; i < nglyphs; i++)
 +    for (i = 0; i < nglyphs; i++) {
          AddGlyph(glyphSet, glyphs[i].glyph, glyphs[i].id);
 +        FreeGlyph(glyphs[i].glyph, glyphSet->fdepth);
 +    }
      if (glyphsBase != glyphsLocal)
          free(glyphsBase);
@@ -1179,9 +1182,13 @@ ProcRenderAddGlyphs(ClientPtr client)
          FreePicture((void *) pSrc, 0);
      if (pSrcPix)
          FreeScratchPixmapHeader(pSrcPix);
 -    for (i = 0; i < nglyphs; i++)
 -        if (glyphs[i].glyph && !glyphs[i].found)
 -            free(glyphs[i].glyph);
 +    for (i = 0; i < nglyphs; i++) {
 +        if (glyphs[i].glyph) {
 +            --glyphs[i].glyph->refcnt;
 +            if (!glyphs[i].found)
 +                free(glyphs[i].glyph);
 +        }
 +    }
      if (glyphsBase != glyphsLocal)
          free(glyphsBase);
      return err;