Commit | Line | Data |
---|---|---|
7217e0ca ML |
1 | From 6070d07a25b33e9630f8dbbde1d1df6f25164286 Mon Sep 17 00:00:00 2001 |
2 | From: Adam Jackson <ajax@redhat.com> | |
3 | Date: Mon, 10 Nov 2014 12:13:43 -0500 | |
4 | Subject: [PATCH 27/33] glx: Length checking for RenderLarge requests (v2) | |
5 | [CVE-2014-8098 3/8] | |
6 | ||
7 | This is a half-measure until we start passing request length into the | |
8 | varsize function, but it's better than the nothing we had before. | |
9 | ||
10 | v2: Verify that there's at least a large render header's worth of | |
11 | dataBytes (Julien Cristau) | |
12 | ||
13 | Reviewed-by: Michal Srb <msrb@suse.com> | |
14 | Reviewed-by: Andy Ritger <aritger@nvidia.com> | |
15 | Signed-off-by: Adam Jackson <ajax@redhat.com> | |
16 | Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com> | |
17 | --- | |
18 | glx/glxcmds.c | 57 ++++++++++++++++++++++++++++++++++----------------------- | |
19 | 1 file changed, 34 insertions(+), 23 deletions(-) | |
20 | ||
21 | Index: xorg-server-1.15.1/glx/glxcmds.c | |
22 | =================================================================== | |
23 | --- xorg-server-1.15.1.orig/glx/glxcmds.c 2014-12-04 11:56:22.749376887 -0500 | |
24 | +++ xorg-server-1.15.1/glx/glxcmds.c 2014-12-04 11:56:22.745376862 -0500 | |
25 | @@ -2099,6 +2099,8 @@ | |
26 | ||
27 | __GLX_DECLARE_SWAP_VARIABLES; | |
28 | ||
29 | + REQUEST_AT_LEAST_SIZE(xGLXRenderLargeReq); | |
30 | + | |
31 | req = (xGLXRenderLargeReq *) pc; | |
32 | if (client->swapped) { | |
33 | __GLX_SWAP_SHORT(&req->length); | |
34 | @@ -2114,12 +2116,14 @@ | |
35 | __glXResetLargeCommandStatus(cl); | |
36 | return error; | |
37 | } | |
38 | + if (safe_pad(req->dataBytes) < 0) | |
39 | + return BadLength; | |
40 | dataBytes = req->dataBytes; | |
41 | ||
42 | /* | |
43 | ** Check the request length. | |
44 | */ | |
45 | - if ((req->length << 2) != __GLX_PAD(dataBytes) + sz_xGLXRenderLargeReq) { | |
46 | + if ((req->length << 2) != safe_pad(dataBytes) + sz_xGLXRenderLargeReq) { | |
47 | client->errorValue = req->length; | |
48 | /* Reset in case this isn't 1st request. */ | |
49 | __glXResetLargeCommandStatus(cl); | |
50 | @@ -2129,7 +2133,7 @@ | |
51 | ||
52 | if (cl->largeCmdRequestsSoFar == 0) { | |
53 | __GLXrenderSizeData entry; | |
54 | - int extra; | |
55 | + int extra = 0; | |
56 | size_t cmdlen; | |
57 | int err; | |
58 | ||
59 | @@ -2142,13 +2146,17 @@ | |
60 | return __glXError(GLXBadLargeRequest); | |
61 | } | |
62 | ||
63 | + if (dataBytes < __GLX_RENDER_LARGE_HDR_SIZE) | |
64 | + return BadLength; | |
65 | + | |
66 | hdr = (__GLXrenderLargeHeader *) pc; | |
67 | if (client->swapped) { | |
68 | __GLX_SWAP_INT(&hdr->length); | |
69 | __GLX_SWAP_INT(&hdr->opcode); | |
70 | } | |
71 | - cmdlen = hdr->length; | |
72 | opcode = hdr->opcode; | |
73 | + if ((cmdlen = safe_pad(hdr->length)) < 0) | |
74 | + return BadLength; | |
75 | ||
76 | /* | |
77 | ** Check for core opcodes and grab entry data. | |
78 | @@ -2170,17 +2178,13 @@ | |
79 | if (extra < 0) { | |
80 | return BadLength; | |
81 | } | |
82 | - /* large command's header is 4 bytes longer, so add 4 */ | |
83 | - if (cmdlen != __GLX_PAD(entry.bytes + 4 + extra)) { | |
84 | - return BadLength; | |
85 | - } | |
86 | } | |
87 | - else { | |
88 | - /* constant size command */ | |
89 | - if (cmdlen != __GLX_PAD(entry.bytes + 4)) { | |
90 | - return BadLength; | |
91 | - } | |
92 | + | |
93 | + /* the +4 is safe because we know entry.bytes is small */ | |
94 | + if (cmdlen != safe_pad(safe_add(entry.bytes + 4, extra))) { | |
95 | + return BadLength; | |
96 | } | |
97 | + | |
98 | /* | |
99 | ** Make enough space in the buffer, then copy the entire request. | |
100 | */ | |
101 | @@ -2207,6 +2211,7 @@ | |
102 | ** We are receiving subsequent (i.e. not the first) requests of a | |
103 | ** multi request command. | |
104 | */ | |
105 | + int bytesSoFar; /* including this packet */ | |
106 | ||
107 | /* | |
108 | ** Check the request number and the total request count. | |
109 | @@ -2225,11 +2230,18 @@ | |
110 | /* | |
111 | ** Check that we didn't get too much data. | |
112 | */ | |
113 | - if ((cl->largeCmdBytesSoFar + dataBytes) > cl->largeCmdBytesTotal) { | |
114 | + if ((bytesSoFar = safe_add(cl->largeCmdBytesSoFar, dataBytes)) < 0) { | |
115 | client->errorValue = dataBytes; | |
116 | __glXResetLargeCommandStatus(cl); | |
117 | return __glXError(GLXBadLargeRequest); | |
118 | } | |
119 | + | |
120 | + if (bytesSoFar > cl->largeCmdBytesTotal) { | |
121 | + client->errorValue = dataBytes; | |
122 | + __glXResetLargeCommandStatus(cl); | |
123 | + return __glXError(GLXBadLargeRequest); | |
124 | + } | |
125 | + | |
126 | memcpy(cl->largeCmdBuf + cl->largeCmdBytesSoFar, pc, dataBytes); | |
127 | cl->largeCmdBytesSoFar += dataBytes; | |
128 | cl->largeCmdRequestsSoFar++; | |
129 | @@ -2241,17 +2253,16 @@ | |
130 | ** This is the last request; it must have enough bytes to complete | |
131 | ** the command. | |
132 | */ | |
133 | - /* NOTE: the two pad macros have been added below; they are needed | |
134 | - ** because the client library pads the total byte count, but not | |
135 | - ** the per-request byte counts. The Protocol Encoding says the | |
136 | - ** total byte count should not be padded, so a proposal will be | |
137 | - ** made to the ARB to relax the padding constraint on the total | |
138 | - ** byte count, thus preserving backward compatibility. Meanwhile, | |
139 | - ** the padding done below fixes a bug that did not allow | |
140 | - ** large commands of odd sizes to be accepted by the server. | |
141 | + /* NOTE: the pad macro below is needed because the client library | |
142 | + ** pads the total byte count, but not the per-request byte counts. | |
143 | + ** The Protocol Encoding says the total byte count should not be | |
144 | + ** padded, so a proposal will be made to the ARB to relax the | |
145 | + ** padding constraint on the total byte count, thus preserving | |
146 | + ** backward compatibility. Meanwhile, the padding done below | |
147 | + ** fixes a bug that did not allow large commands of odd sizes to | |
148 | + ** be accepted by the server. | |
149 | */ | |
150 | - if (__GLX_PAD(cl->largeCmdBytesSoFar) != | |
151 | - __GLX_PAD(cl->largeCmdBytesTotal)) { | |
152 | + if (safe_pad(cl->largeCmdBytesSoFar) != cl->largeCmdBytesTotal) { | |
153 | client->errorValue = dataBytes; | |
154 | __glXResetLargeCommandStatus(cl); | |
155 | return __glXError(GLXBadLargeRequest); |