spi: spidev: only use up TX/RX bounce buffer space when needed
Hi Ian,
The patch 9a12bff7c346: "spi: spidev: only use up TX/RX bounce buffer space when needed" from Feb 16, 2015, has a potential integer overflow issue.
drivers/spi/spidev.c
241 total = 0;
242 tx_total = 0;
243 rx_total = 0;
244 for (n = n_xfers, k_tmp = k_xfers, u_tmp = u_xfers;
245 n;
246 n--, k_tmp++, u_tmp++) {
247 k_tmp->len = u_tmp->len;
248
249 total += k_tmp->len;
^^^^^^^^^^^^^^^^^^^This is a potential integer overflow but the impact is not serious.
250 /* Since the function returns the total length of transfers
251 * on success, restrict the total to positive int values to
252 * avoid the return value looking like an error.
253 */
254 if (total > INT_MAX) {
255 status = -EMSGSIZE;
256 goto done;
257 }
258
259 if (u_tmp->rx_buf) {
260 /* this transfer needs space in RX bounce buffer */
261 rx_total += k_tmp->len;
^^^^^^^^^^^^^^^^^^^^^^This one can maybe result in an info leak? I'm not sure.
262 if (rx_total > bufsiz) {
263 status = -EMSGSIZE;
264 goto done;
265 }
266 k_tmp->rx_buf = rx_buf;
267 if (!access_ok(VERIFY_WRITE, (u8 __user *)
268 (uintptr_t) u_tmp->rx_buf,
269 u_tmp->len))
270 goto done;
271 rx_buf += k_tmp->len;
272 }regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 17 lines · Dan Carpenter
Hi Ian,
The patch
9a12bff7c346: "spi: spidev: only use up TX/RX bounce buffer space when needed" from Feb 16, 2015, has a potential integer overflow issue.
drivers/spi/spidev.c241 total = 0; 242 tx_total = 0; 243 rx_total = 0; 244 for (n = n_xfers, k_tmp = k_xfers, u_tmp = u_xfers; 245 n; 246 n--, k_tmp++, u_tmp++) { 247 k_tmp->len = u_tmp->len; 248 249 total += k_tmp->len; ^^^^^^^^^^^^^^^^^^^
Quoted 1 line
This is a potential integer overflow but the impact is not serious.
It's possible, although the previous version also had the same problem.
It's quite serious as the SPI core may read past the end of the
pre-allocated rx buffer and write past the end of the pre-allocated tx buffer.
Quoted 6 lines
250 /* Since the function returns the total length of transfers
251 * on success, restrict the total to positive int values to
252 * avoid the return value looking like an error.
253 */
254 if (total >INT_MAX) {
Changing that to if (total > INT_MAX || k_tmp->len > INT_MAX) would fix the overflow. I'll send a patch later today.
Quoted 9 lines
255 status = -EMSGSIZE; 256 goto done; 257 } 258 259 if (u_tmp->rx_buf) { 260 /* this transfer needs space in RX bounce buffer */ 261 rx_total += k_tmp->len; ^^^^^^^^^^^^^^^^^^^^^^This one can maybe result in an info leak? I'm not sure.
This is reserving space in the pre-allocated rx buffer which should be fine as long as the previous integer overflow is fixed. After the call to spi_sync(), there is a possibility of copying old data from the SPI device to the user if the SPI message was not fully transferred. If that's a problem, I think the safest fix would be to clear the pre-allocated rx buffer (at least the first rx_total bytes) before calling spi_sync(). Alternatively, the code that copies the rx data back to the user could stop copying when the returned message length is exceeded, and either clear the remaining user rx buffer space or leave it unchanged. I guess that's open to discussion.
Quoted 13 lines
262 if (rx_total > bufsiz) {
263 status = -EMSGSIZE;
264 goto done;
265 }
266 k_tmp->rx_buf = rx_buf;
267 if (!access_ok(VERIFY_WRITE, (u8 __user *)
268 (uintptr_t) u_tmp->rx_buf,
269 u_tmp->len))
270 goto done;
271 rx_buf += k_tmp->len;
272 }Quoted 3 lines
regards,
dan carpenter
Best regards,
Ian Abbott
Signature · 6 lines
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti-GUHe90Wt2aFaa/[email protected]> )=- -=( Web: http://www.mev.co.uk/ )=- -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html