[PATCH] xkb: Don't swap XkbSetGeometry data in the input buffer

[Peter Hutterer]

On Fri, Jan 16, 2015 at 08:08:59PM +0100, Olivier Fourdan wrote:
> The XkbSetGeometry request embeds data which needs to be swapped when the
> server and the client have different endianess.
> 
> _XkbSetGeometry() invokes functions that swap these data directly in the
> input buffer.
> 
> However, ProcXkbSetGeometry() may call _XkbSetGeometry() more than once
> (if there is more than one keyboard), thus causing on swapped clients the
> same data to be swapped twice in memory, further causing a server crash
> because the strings lengths on the second time are way off bounds.
> 
> To allow _XkbSetGeometry() to run reliably more than once with swapped
> clients, do not swap the data in the buffer, use variables instead.
> 
> Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>

Keith, please merge this one directly, thanks.

Cheers,
   Peter

> ---
>  xkb/xkb.c | 35 +++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/xkb/xkb.c b/xkb/xkb.c
> index 15c7f34..b9a3ac4 100644
> --- a/xkb/xkb.c
> +++ b/xkb/xkb.c
> @@ -4961,14 +4961,13 @@ static char *
>  _GetCountedString(char **wire_inout, Bool swap)
>  {
>      char *wire, *str;
> -    CARD16 len, *plen;
> +    CARD16 len;
>  
>      wire = *wire_inout;
> -    plen = (CARD16 *) wire;
> +    len = *(CARD16 *) wire;
>      if (swap) {
> -        swaps(plen);
> +        swaps(&len);
>      }
> -    len = *plen;
>      str = malloc(len + 1);
>      if (str) {
>          memcpy(str, &wire[2], len);
> @@ -4985,25 +4984,28 @@ _CheckSetDoodad(char **wire_inout,
>  {
>      char *wire;
>      xkbDoodadWireDesc *dWire;
> +    xkbAnyDoodadWireDesc any;
> +    xkbTextDoodadWireDesc text;
>      XkbDoodadPtr doodad;
>  
>      dWire = (xkbDoodadWireDesc *) (*wire_inout);
> +    any = dWire->any;
>      wire = (char *) &dWire[1];
>      if (client->swapped) {
> -        swapl(&dWire->any.name);
> -        swaps(&dWire->any.top);
> -        swaps(&dWire->any.left);
> -        swaps(&dWire->any.angle);
> +        swapl(&any.name);
> +        swaps(&any.top);
> +        swaps(&any.left);
> +        swaps(&any.angle);
>      }
>      CHK_ATOM_ONLY(dWire->any.name);
> -    doodad = XkbAddGeomDoodad(geom, section, dWire->any.name);
> +    doodad = XkbAddGeomDoodad(geom, section, any.name);
>      if (!doodad)
>          return BadAlloc;
>      doodad->any.type = dWire->any.type;
>      doodad->any.priority = dWire->any.priority;
> -    doodad->any.top = dWire->any.top;
> -    doodad->any.left = dWire->any.left;
> -    doodad->any.angle = dWire->any.angle;
> +    doodad->any.top = any.top;
> +    doodad->any.left = any.left;
> +    doodad->any.angle = any.angle;
>      switch (doodad->any.type) {
>      case XkbOutlineDoodad:
>      case XkbSolidDoodad:
> @@ -5026,12 +5028,13 @@ _CheckSetDoodad(char **wire_inout,
>                                                dWire->text.colorNdx);
>              return BadMatch;
>          }
> +        text = dWire->text;
>          if (client->swapped) {
> -            swaps(&dWire->text.width);
> -            swaps(&dWire->text.height);
> +            swaps(&text.width);
> +            swaps(&text.height);
>          }
> -        doodad->text.width = dWire->text.width;
> -        doodad->text.height = dWire->text.height;
> +        doodad->text.width = text.width;
> +        doodad->text.height = text.height;
>          doodad->text.color_ndx = dWire->text.colorNdx;
>          doodad->text.text = _GetCountedString(&wire, client->swapped);
>          doodad->text.font = _GetCountedString(&wire, client->swapped);
> -- 
> 2.1.0
>