* re: spi: spidev: only use up TX/RX bounce buffer space when needed
@ 2015-03-23 13:01 Dan Carpenter
2015-03-23 15:53 ` Ian Abbott
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2015-03-23 13:01 UTC (permalink / raw)
To: abbotti-GUHe90Wt2aFaa/9Udqfwiw; +Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: spi: spidev: only use up TX/RX bounce buffer space when needed
2015-03-23 13:01 spi: spidev: only use up TX/RX bounce buffer space when needed Dan Carpenter
@ 2015-03-23 15:53 ` Ian Abbott
0 siblings, 0 replies; 2+ messages in thread
From: Ian Abbott @ 2015-03-23 15:53 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA
On 23/03/15 13:01, Dan Carpenter wrote:
> 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.
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.
>
> 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.
> 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.
>
> 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
>
Best regards,
Ian Abbott
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti-GUHe90Wt2aFaa/9Udqfwiw@public.gmane.org> )=-
-=( Web: http://www.mev.co.uk/ )=-
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-03-23 15:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-23 13:01 spi: spidev: only use up TX/RX bounce buffer space when needed Dan Carpenter
2015-03-23 15:53 ` Ian Abbott
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).