usb: usbip: fix integer overflow in usbip_recv_iso()
Subsystems (2)
PATCH 1/3 usb: usbip: fix integer overflow in usbip_recv_iso()
usbip_recv_iso() computes the iso descriptor buffer size as:
int size = np * sizeof(*iso);
where np comes straight from the wire (urb->number_of_packets, set by usbip_pack_ret_submit() before we get here). With np = 0x10000001 and sizeof(*iso) == 16 the product is 0x100000010 which truncates to 16 on a 32-bit int. kzalloc(16) succeeds but the following receive loop writes np * 16 bytes into it - game over.
USBIP_MAX_ISO_PACKETS (1024) already exists in usbip_common.h for the submit path but was never enforced on the receive side.
Clamp np to [1, USBIP_MAX_ISO_PACKETS] and switch to kmalloc_array() so the allocator itself can catch overflows in the future.
One subtlety: usbip_pack_ret_submit() already copied the bogus np into urb->number_of_packets before we run, so just returning -EPROTO isn't enough - processcompl() in the HCD will still iterate that many iso_frame_desc entries when it completes the failed URB. Zero out urb->number_of_packets before bailing to prevent that secondary crash (confirmed on 6.12.0, processcompl+0x63 with CR2 in unmapped slab).
--- drivers/usb/usbip/usbip_common.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/usb/usbip/usbip_common.c +13 -4
@@ -662,7 +662,6 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)void *buff;struct usbip_iso_packet_descriptor *iso;int np = urb->number_of_packets; -int size = np *sizeof (*iso);int i;int ret;int total_length =0 ;
@@ -674,12 +673,22 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)if (np ==0 )return 0 ; - buff =kzalloc (size,GFP_KERNEL ); +if (np <0 || np >USBIP_MAX_ISO_PACKETS ) { +dev_err (&urb->dev->dev, +"recv iso: invalid number_of_packets %d\n" , np); +/* usbip_pack_ret_submit() already set urb->number_of_packets +* from the wire - zero it so processcompl() does not iterate +* OOB descriptors on the way out. */ + urb->number_of_packets =0 ; +return -EPROTO ; + } + + buff =kmalloc_array (np,sizeof (*iso),GFP_KERNEL );if (!buff)return -ENOMEM ; - ret =usbip_recv (ud->tcp_socket, buff, size); -if (ret != size) { + ret =usbip_recv (ud->tcp_socket, buff, np *sizeof (*iso)); +if (ret != np * (int )sizeof (*iso)) {dev_err (&urb->dev->dev,"recv iso_frame_descriptor, %d\n" , ret);kfree (buff);
Signature · 1 line
2.34.1
PATCH 2/3 usb: usbip: validate iso frame actual_length in usbip_recv_iso()
usbip_recv_iso() sums each frame's actual_length into an int accumulator without checking the individual values first:
total_length += urb->iso_frame_desc[i].actual_length;
A malicious server can send actual_length = 0xFFFFFFFC for one frame and a small value for the other, making the signed sum wrap around to match urb->actual_length. The sanity check passes, and usbip_pad_iso() later computes a negative actualoffset, feeding it to memmove() as a source pointer - reads before the allocation, leaked to userspace via USBDEVFS_REAPURB.
Reject any frame whose actual_length exceeds transfer_buffer_length (one frame can't carry more data than the whole buffer), and widen the accumulator to u32 so that many moderately-large frames can't wrap it either.
--- drivers/usb/usbip/usbip_common.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/usb/usbip/usbip_common.c +12 -3
@@ -664,7 +664,7 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)int np = urb->number_of_packets;int i;int ret; -int total_length =0 ; +u32 total_length =0 ;if (!usb_pipeisoc (urb->pipe))return 0 ;
@@ -705,14 +705,23 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)for (i =0 ; i < np; i++) {usbip_iso_packet_correct_endian (&iso[i],0 );usbip_pack_iso (&iso[i], &urb->iso_frame_desc[i],0 ); +if (urb->iso_frame_desc[i].actual_length > + (unsigned int )urb->transfer_buffer_length) { +dev_err (&urb->dev->dev, +"recv iso: frame actual_length %u exceeds buffer %d\n" , + urb->iso_frame_desc[i].actual_length, + urb->transfer_buffer_length); +kfree (buff); +return -EPROTO ; + } total_length += urb->iso_frame_desc[i].actual_length; }kfree (buff); -if (total_length != urb->actual_length) { +if (total_length != (u32 )urb->actual_length) {dev_err (&urb->dev->dev, -"total length of iso packets %d not equal to actual length of buffer %d\n" , +"total length of iso packets %u not equal to actual length of buffer %d\n" , total_length, urb->actual_length);if (ud->side ==USBIP_STUB || ud->side ==USBIP_VUDC )
Signature · 1 line
2.34.1
PATCH 3/3 usb: usbip: fix OOB read/write in usbip_pad_iso()
In reply to Kelvin Mbogousbip_pad_iso() repositions ISO frame data within the transfer buffer via memmove(). Neither the source offset (actualoffset, derived by subtracting wire-supplied actual_length values) nor the destination offset (iso_frame_desc[i].offset, taken directly from the wire) is bounds-checked.
If a crafted actual_length wraps actualoffset negative through the subtraction (see patch 2/3 for the root cause), the memmove source points before the allocation - slab OOB read, data returned to userspace.
Independently, iso_frame_desc[i].offset is never validated against transfer_buffer_length. Setting offset past the end of the buffer gives a fully controlled OOB write into whatever sits next in the slab - confirmed with offset=400 on a 392-byte buffer, 64-byte write.
Add bounds checks for both the source and destination ranges before each memmove call. Use unsigned comparisons after the sign check on actualoffset to avoid signed/unsigned conversion surprises.
--- drivers/usb/usbip/usbip_common.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
drivers/usb/usbip/usbip_common.c +30 -0
@@ -769,6 +769,36 @@ void usbip_pad_iso(struct usbip_device *ud, struct urb *urb)*/ for (i = np-1 ; i >0 ; i--) { actualoffset -= urb->iso_frame_desc[i].actual_length; + +/* source: actualoffset can go negative via crafted +* actual_length - catch that plus any overshoot */ +if (actualoffset <0 || + (unsigned int )actualoffset > (unsigned int )urb->transfer_buffer_length || + urb->iso_frame_desc[i].actual_length > + (unsigned int )urb->transfer_buffer_length - + (unsigned int )actualoffset) { +dev_err (&urb->dev->dev, +"pad_iso: bad src off=%d len=%u bufsz=%d\n" , + actualoffset, + urb->iso_frame_desc[i].actual_length, + urb->transfer_buffer_length); +return ; + } + +/* dest: offset comes straight from the wire, never checked */ +if (urb->iso_frame_desc[i].offset > + (unsigned int )urb->transfer_buffer_length || + urb->iso_frame_desc[i].actual_length > + (unsigned int )urb->transfer_buffer_length - + urb->iso_frame_desc[i].offset) { +dev_err (&urb->dev->dev, +"pad_iso: bad dst off=%u len=%u bufsz=%d\n" , + urb->iso_frame_desc[i].offset, + urb->iso_frame_desc[i].actual_length, + urb->transfer_buffer_length); +return ; + } +memmove (urb->transfer_buffer + urb->iso_frame_desc[i].offset, urb->transfer_buffer + actualoffset, urb->iso_frame_desc[i].actual_length);
Signature · 1 line
2.34.1
Greg KH
maintainerRe: [PATCH 3/3] usb: usbip: fix OOB read/write in usbip_pad_iso()
Quoted 32 lines · Kelvin Mbogo
usbip_pad_iso()repositions ISO frame data within the transfer buffer viamemmove(). Neither the source offset (actualoffset, derived by subtracting wire-suppliedactual_lengthvalues) nor the destination offset (iso_frame_desc[i].offset, taken directly from the wire) is bounds-checked.If a crafted
actual_lengthwraps actualoffset negative through the subtraction (see patch 2/3 for the root cause), the memmove source points before the allocation - slab OOB read, data returned to userspace.Independently, iso_frame_desc[i].offset is never validated against
transfer_buffer_length. Setting offset past the end of the buffer gives a fully controlled OOB write into whatever sits next in the slab - confirmed with offset=400 on a 392-byte buffer, 64-byte write.Add bounds checks for both the source and destination ranges before each memmove call. Use unsigned comparisons after the sign check on actualoffset to avoid signed/unsigned conversion surprises.
Reported-by Kelvin Mbogo <[email protected]>Signed-off-by Kelvin Mbogo <[email protected]>--- drivers/usb/usbip/usbip_common.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)drivers/usb/usbip/usbip_common.c +0
-0@@ -769,6 +769,36 @@ void usbip_pad_iso(struct usbip_device *ud, struct urb *urb)*/
drivers/usb/usbip/usbip_common.c void usbip_pad_iso(struct usbip_device *ud, struct urb *urb) · line 779
Quoted 10 lines
drivers/usb/usbip/usbip_common.c +8
-0for (i = np-1 ; i >0 ; i--) { actualoffset -= urb->iso_frame_desc[i].actual_length; + +/* source: actualoffset can go negative via crafted +* actual_length - catch that plus any overshoot */ +if (actualoffset <0 || + (unsigned int )actualoffset > (unsigned int )urb->transfer_buffer_length || + urb->iso_frame_desc[i].actual_length > + (unsigned int )urb->transfer_buffer_length - + (unsigned int )actualoffset) {
That's soem rough indentation, didn't checkpatch complain about it?
drivers/usb/usbip/usbip_common.c void usbip_pad_iso(struct usbip_device *ud, struct urb *urb) · line 788
Quoted 9 lines
drivers/usb/usbip/usbip_common.c +9
-0+dev_err (&urb->dev->dev, +"pad_iso: bad src off=%d len=%u bufsz=%d\n" , + actualoffset, + urb->iso_frame_desc[i].actual_length, + urb->transfer_buffer_length); +return ; + } + +/* dest: offset comes straight from the wire, never checked */
as you are checking it here, why not say that?
drivers/usb/usbip/usbip_common.c void usbip_pad_iso(struct usbip_device *ud, struct urb *urb) · line 793
Quoted 5 lines
drivers/usb/usbip/usbip_common.c +5
-0+if (urb->iso_frame_desc[i].offset > + (unsigned int )urb->transfer_buffer_length || + urb->iso_frame_desc[i].actual_length > + (unsigned int )urb->transfer_buffer_length - + urb->iso_frame_desc[i].offset) {
Again, odd indentation.
thanks,
greg k-h
Greg KH
maintainerQuoted 23 lines · Kelvin Mbogo
usbip_recv_iso()computes the iso descriptor buffer size as:int size = np * sizeof(*iso);where np comes straight from the wire (
urb->number_of_packets, set byusbip_pack_ret_submit()before we get here). With np =0x10000001andsizeof(*iso)== 16 the product is0x100000010which truncates to 16 on a 32-bit int. kzalloc(16) succeeds but the following receive loop writes np * 16 bytes into it - game over.
USBIP_MAX_ISO_PACKETS(1024) already exists inusbip_common.hfor the submit path but was never enforced on the receive side.Clamp np to [1,
USBIP_MAX_ISO_PACKETS] and switch tokmalloc_array()so the allocator itself can catch overflows in the future.One subtlety:
usbip_pack_ret_submit()already copied the bogus np intourb->number_of_packetsbefore we run, so just returning-EPROTOisn't enough -processcompl()in the HCD will still iterate that manyiso_frame_descentries when it completes the failed URB. Zero outurb->number_of_packetsbefore bailing to prevent that secondary crash (confirmed on 6.12.0,processcompl+0x63with CR2 in unmapped slab).
Quoted 2 lines
Reported-by Kelvin Mbogo <[email protected]>Signed-off-by Kelvin Mbogo <[email protected]>
Nit, no need to have reported-by when you author and sign off on a patch.
and as this is public, no need to cc: [email protected] anymore either.
Quoted 20 lines
--- drivers/usb/usbip/usbip_common.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)drivers/usb/usbip/usbip_common.c +0
-1@@ -662,7 +662,6 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)void *buff;struct usbip_iso_packet_descriptor *iso;int np = urb->number_of_packets; -int size = np *sizeof (*iso);int i;int ret;int total_length =0 ;@@ -674,12 +673,22 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)if (np ==0 )return 0 ;
drivers/usb/usbip/usbip_common.c int usbip_recv_iso(struct usbip_device *ud, struct urb *urb) · line 684
Quoted 10 lines
drivers/usb/usbip/usbip_common.c +9
-1- buff =kzalloc (size,GFP_KERNEL ); +if (np <0 || np >USBIP_MAX_ISO_PACKETS ) { +dev_err (&urb->dev->dev, +"recv iso: invalid number_of_packets %d\n" , np); +/* usbip_pack_ret_submit() already set urb->number_of_packets +* from the wire - zero it so processcompl() does not iterate +* OOB descriptors on the way out. */ + urb->number_of_packets =0 ; +return -EPROTO ; + }
Why not just make the np == 0 do the same here and just silently eat the message? Do we have to report an error?
Also, nit, the comment style you used here is for network drivers, not USB drivers :)
drivers/usb/usbip/usbip_common.c int usbip_recv_iso(struct usbip_device *ud, struct urb *urb) · line 686
Quoted 2 lines
drivers/usb/usbip/usbip_common.c +2
-0+ + buff =kmalloc_array (np,sizeof (*iso),GFP_KERNEL );
Not kzalloc_objs()?
drivers/usb/usbip/usbip_common.c int usbip_recv_iso(struct usbip_device *ud, struct urb *urb) · line 691
Quoted 7 lines
drivers/usb/usbip/usbip_common.c +2
-2if (!buff)return -ENOMEM ; - ret =usbip_recv (ud->tcp_socket, buff, size); -if (ret != size) { + ret =usbip_recv (ud->tcp_socket, buff, np *sizeof (*iso)); +if (ret != np * (int )sizeof (*iso)) {
is the cast needed? Or is there a compiler warning without it?
And as you are calculating "np * sizeof(*iso)" multiple times here, why not just keep the "size" variable at the top here? Then that would be one less line changed here.
thanks,
greg k-h