staging: gdm724x: check for buffer overflow in gdm_lte_multi_sdu_pkt()
Subsystems (2)
PATCH 1/2 staging: gdm724x: check for buffer overflow in gdm_lte_multi_sdu_pkt()
There needs to be a check to verify that we don't read beyond the end of "buf". This function is called from do_rx(). The "buf" is the USB transfer_buffer and "len" is "urb->actual_length".
---
The truth is that I don't really understand where the 12 comes from in (hci_len - 12) but I copy and pasted it. I am a bit tempted to say that actually hci_len - 12 has to be greater than zero but I didn't do that because I don't have the hardware to test it.
drivers/staging/gdm724x/gdm_lte.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/staging/gdm724x/gdm_lte.c +9 -1
@@ -677,6 +677,7 @@ static void gdm_lte_multi_sdu_pkt(struct phy_dev *phy_dev, char *buf, int len)struct sdu *sdu =NULL ;u8 endian = phy_dev->get_endian (phy_dev->priv_dev);u8 *data = (u8 *)multi_sdu->data; +int copied;u16 i =0 ;u16 num_packet;u16 hci_len;
@@ -688,6 +689,12 @@ static void gdm_lte_multi_sdu_pkt(struct phy_dev *phy_dev, char *buf, int len) num_packet =gdm_dev16_to_cpu (endian, multi_sdu->num_packet);for (i =0 ; i < num_packet; i++) { + copied = data - multi_sdu->data; +if (len < copied +sizeof (*sdu)) { +pr_err ("rx prevent buffer overflow" ); +return ; + } + sdu = (struct sdu *)data; cmd_evt =gdm_dev16_to_cpu (endian, sdu->cmd_evt);
@@ -698,7 +705,8 @@ static void gdm_lte_multi_sdu_pkt(struct phy_dev *phy_dev, char *buf, int len)pr_err ("rx sdu wrong hci %04x\n" , cmd_evt);return ; } -if (hci_len <12 ) { +if (hci_len <12 || + len < copied +sizeof (*sdu) + (hci_len -12 )) {pr_err ("rx sdu invalid len %d\n" , hci_len);return ; }
Signature · 1 line
2.30.2
PATCH 2/2 staging: gdm724x: check for overflow in gdm_lte_netif_rx()
This code assumes that "len" is at least 62 bytes, but we need a check to prevent a read overflow.
---
There is a different place that does:
ip_version = buf[0] >> 4;
Which assumes that "len" is one. I think that should be checked in the caller... I didn't add a check here because I think it's the wrong place but then I didn't add a check in the caller because I wasn't able to test it. I'm not really that worried about reading one element beyond the end of the buffer. If we get extremely unlucky it could result in a crash... Hopefully we will remember to look at this again before moving this code out of staging.
#TODO:
drivers/staging/gdm724x/gdm_lte.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/staging/gdm724x/gdm_lte.c +6 -4
@@ -611,10 +611,12 @@ static void gdm_lte_netif_rx(struct net_device *dev, char *buf,* bytes (99,130,83,99 dec) */ } __packed; -void *addr = buf +sizeof (struct iphdr ) + -sizeof (struct udphdr ) + - offsetof(struct dhcp_packet , chaddr); -ether_addr_copy (nic->dest_mac_addr, addr); +int offset =sizeof (struct iphdr ) + +sizeof (struct udphdr ) + + offsetof(struct dhcp_packet , chaddr); +if (offset +ETH_ALEN > len) +return ; +ether_addr_copy (nic->dest_mac_addr, buf + offset); } }
Signature · 1 line
2.30.2