[PATCH v2 xserver] record: Prevent out of bounds access when recording a reply.
Jeremy Huddleston
jeremyhu at apple.com
Sun Nov 6 16:56:38 PST 2011
This seems to have fallen off the radar. I've pulled it into my tree, so it'll eventually be pulled into master.
--Jeremy
On Oct 4, 2011, at 2:25 AM, Rami Ylimäki wrote:
> Any pad bytes in replies are written to the client from a zeroed
> array. However, record extension tries to incorrectly access the pad
> bytes from the end of reply data.
>
> Signed-off-by: Rami Ylimäki <rami.ylimaki at vincit.fi>
> Reviewed-by: Erkki Seppälä <erkki.seppala at vincit.fi>
> ---
> v1: Fixed the issue when a reply was cached into a recording context buffer.
> v2: Fixes the issue also for large replies that don't fit into the cache.
>
> include/os.h | 3 ++-
> os/io.c | 1 +
> record/record.c | 53 ++++++++++++++++++++++++++++++-----------------------
> 3 files changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/include/os.h b/include/os.h
> index 5401ea4..fda0181 100644
> --- a/include/os.h
> +++ b/include/os.h
> @@ -451,9 +451,10 @@ extern _X_EXPORT CallbackListPtr ReplyCallback;
> typedef struct {
> ClientPtr client;
> const void *replyData;
> - unsigned long dataLenBytes;
> + unsigned long dataLenBytes; /* actual bytes from replyData + pad bytes */
> unsigned long bytesRemaining;
> Bool startOfReply;
> + unsigned long padBytes; /* pad bytes from zeroed array */
> } ReplyInfoRec;
>
> /* stuff for FlushCallback */
> diff --git a/os/io.c b/os/io.c
> index 068f5f0..955bf8b 100644
> --- a/os/io.c
> +++ b/os/io.c
> @@ -809,6 +809,7 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
> replyinfo.client = who;
> replyinfo.replyData = buf;
> replyinfo.dataLenBytes = count + padBytes;
> + replyinfo.padBytes = padBytes;
> if (who->replyBytesRemaining)
> { /* still sending data of an earlier reply */
> who->replyBytesRemaining -= count + padBytes;
> diff --git a/record/record.c b/record/record.c
> index 68311ac..db77b64 100644
> --- a/record/record.c
> +++ b/record/record.c
> @@ -269,8 +269,9 @@ RecordFlushReplyBuffer(
> * device events and EndOfData, pClient is NULL.
> * category is the category of the protocol element, as defined
> * by the RECORD spec.
> - * data is a pointer to the protocol data, and datalen is its length
> - * in bytes.
> + * data is a pointer to the protocol data, and datalen - padlen
> + * is its length in bytes.
> + * padlen is the number of pad bytes from a zeroed array.
> * futurelen is the number of bytes that will be sent in subsequent
> * calls to this function to complete this protocol element.
> * In those subsequent calls, futurelen will be -1 to indicate
> @@ -290,7 +291,7 @@ RecordFlushReplyBuffer(
> */
> static void
> RecordAProtocolElement(RecordContextPtr pContext, ClientPtr pClient,
> - int category, pointer data, int datalen, int futurelen)
> + int category, pointer data, int datalen, int padlen, int futurelen)
> {
> CARD32 elemHeaderData[2];
> int numElemHeaders = 0;
> @@ -398,15 +399,20 @@ RecordAProtocolElement(RecordContextPtr pContext, ClientPtr pClient,
> }
> if (datalen)
> {
> + static char padBuffer[3]; /* as in FlushClient */
> memcpy(pContext->replyBuffer + pContext->numBufBytes,
> - data, datalen);
> - pContext->numBufBytes += datalen;
> + data, datalen - padlen);
> + pContext->numBufBytes += datalen - padlen;
> + memcpy(pContext->replyBuffer + pContext->numBufBytes,
> + padBuffer, padlen);
> + pContext->numBufBytes += padlen;
> }
> }
> else
> + {
> RecordFlushReplyBuffer(pContext, (pointer)elemHeaderData,
> - numElemHeaders, (pointer)data, datalen);
> -
> + numElemHeaders, (pointer)data, datalen - padlen);
> + }
> } /* RecordAProtocolElement */
>
>
> @@ -483,19 +489,19 @@ RecordABigRequest(RecordContextPtr pContext, ClientPtr client, xReq *stuff)
> /* record the request header */
> bytesLeft = client->req_len << 2;
> RecordAProtocolElement(pContext, client, XRecordFromClient,
> - (pointer)stuff, SIZEOF(xReq), bytesLeft);
> + (pointer)stuff, SIZEOF(xReq), 0, bytesLeft);
>
> /* reinsert the extended length field that was squished out */
> bigLength = client->req_len + bytes_to_int32(sizeof(bigLength));
> if (client->swapped)
> swapl(&bigLength);
> RecordAProtocolElement(pContext, client, XRecordFromClient,
> - (pointer)&bigLength, sizeof(bigLength), /* continuation */ -1);
> + (pointer)&bigLength, sizeof(bigLength), 0, /* continuation */ -1);
> bytesLeft -= sizeof(bigLength);
>
> /* record the rest of the request after the length */
> RecordAProtocolElement(pContext, client, XRecordFromClient,
> - (pointer)(stuff + 1), bytesLeft, /* continuation */ -1);
> + (pointer)(stuff + 1), bytesLeft, 0, /* continuation */ -1);
> } /* RecordABigRequest */
>
>
> @@ -542,7 +548,7 @@ RecordARequest(ClientPtr client)
> RecordABigRequest(pContext, client, stuff);
> else
> RecordAProtocolElement(pContext, client, XRecordFromClient,
> - (pointer)stuff, client->req_len << 2, 0);
> + (pointer)stuff, client->req_len << 2, 0, 0);
> }
> else /* extension, check minor opcode */
> {
> @@ -566,7 +572,7 @@ RecordARequest(ClientPtr client)
> else
> RecordAProtocolElement(pContext, client,
> XRecordFromClient, (pointer)stuff,
> - client->req_len << 2, 0);
> + client->req_len << 2, 0, 0);
> break;
> }
> } /* end for each minor op info */
> @@ -619,7 +625,8 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
> if (pContext->continuedReply)
> {
> RecordAProtocolElement(pContext, client, XRecordFromServer,
> - (pointer)pri->replyData, pri->dataLenBytes, /* continuation */ -1);
> + (pointer)pri->replyData, pri->dataLenBytes,
> + pri->padBytes, /* continuation */ -1);
> if (!pri->bytesRemaining)
> pContext->continuedReply = 0;
> }
> @@ -629,7 +636,7 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
> if (majorop <= 127)
> { /* core reply */
> RecordAProtocolElement(pContext, client, XRecordFromServer,
> - (pointer)pri->replyData, pri->dataLenBytes, pri->bytesRemaining);
> + (pointer)pri->replyData, pri->dataLenBytes, 0, pri->bytesRemaining);
> if (pri->bytesRemaining)
> pContext->continuedReply = 1;
> }
> @@ -651,7 +658,7 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
> {
> RecordAProtocolElement(pContext, client,
> XRecordFromServer, (pointer)pri->replyData,
> - pri->dataLenBytes, pri->bytesRemaining);
> + pri->dataLenBytes, 0, pri->bytesRemaining);
> if (pri->bytesRemaining)
> pContext->continuedReply = 1;
> break;
> @@ -723,7 +730,7 @@ RecordADeliveredEventOrError(CallbackListPtr *pcbl, pointer nulldata, pointer ca
>
> }
> RecordAProtocolElement(pContext, pClient,
> - XRecordFromServer, pEvToRecord, SIZEOF(xEvent), 0);
> + XRecordFromServer, pEvToRecord, SIZEOF(xEvent), 0, 0);
> }
> } /* end for each event */
> } /* end this client is on this context */
> @@ -774,7 +781,7 @@ RecordSendProtocolEvents(RecordClientsAndProtocolPtr pRCAP,
> }
>
> RecordAProtocolElement(pContext, NULL,
> - XRecordFromServer, pEvToRecord, SIZEOF(xEvent), 0);
> + XRecordFromServer, pEvToRecord, SIZEOF(xEvent), 0, 0);
> /* make sure device events get flushed in the absence
> * of other client activity
> */
> @@ -2415,7 +2422,7 @@ ProcRecordEnableContext(ClientPtr client)
> assert(numEnabledContexts > 0);
>
> /* send StartOfData */
> - RecordAProtocolElement(pContext, NULL, XRecordStartOfData, NULL, 0, 0);
> + RecordAProtocolElement(pContext, NULL, XRecordStartOfData, NULL, 0, 0, 0);
> RecordFlushReplyBuffer(pContext, NULL, 0, NULL, 0);
> return Success;
> } /* ProcRecordEnableContext */
> @@ -2446,7 +2453,7 @@ RecordDisableContext(RecordContextPtr pContext)
> if (!pContext->pRecordingClient) return;
> if (!pContext->pRecordingClient->clientGone)
> {
> - RecordAProtocolElement(pContext, NULL, XRecordEndOfData, NULL, 0, 0);
> + RecordAProtocolElement(pContext, NULL, XRecordEndOfData, NULL, 0, 0, 0);
> RecordFlushReplyBuffer(pContext, NULL, 0, NULL, 0);
> /* Re-enable request processing on this connection. */
> AttendClient(pContext->pRecordingClient);
> @@ -2761,7 +2768,7 @@ RecordConnectionSetupInfo(RecordContextPtr pContext, NewClientInfoRec *pci)
> SwapConnSetupPrefix(pci->prefix, (xConnSetupPrefix*)pConnSetup);
> SwapConnSetupInfo((char*)pci->setup, (char*)(pConnSetup + prefixsize));
> RecordAProtocolElement(pContext, pci->client, XRecordClientStarted,
> - (pointer)pConnSetup, prefixsize + restsize, 0);
> + (pointer)pConnSetup, prefixsize + restsize, 0, 0);
> free(pConnSetup);
> }
> else
> @@ -2770,9 +2777,9 @@ RecordConnectionSetupInfo(RecordContextPtr pContext, NewClientInfoRec *pci)
> * data in two pieces
> */
> RecordAProtocolElement(pContext, pci->client, XRecordClientStarted,
> - (pointer)pci->prefix, prefixsize, restsize);
> + (pointer)pci->prefix, prefixsize, 0, restsize);
> RecordAProtocolElement(pContext, pci->client, XRecordClientStarted,
> - (pointer)pci->setup, restsize, /* continuation */ -1);
> + (pointer)pci->setup, restsize, 0, /* continuation */ -1);
> }
> } /* RecordConnectionSetupInfo */
>
> @@ -2849,7 +2856,7 @@ RecordAClientStateChange(CallbackListPtr *pcbl, pointer nulldata, pointer callda
> {
> if (pContext->pRecordingClient && pRCAP->clientDied)
> RecordAProtocolElement(pContext, pClient,
> - XRecordClientDied, NULL, 0, 0);
> + XRecordClientDied, NULL, 0, 0, 0);
> RecordDeleteClientFromRCAP(pRCAP, pos);
> }
> }
> --
> 1.7.1
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
More information about the xorg-devel
mailing list