* [PATCH v6 0/2] net: tulip: add checks to avoid OOB access
@ 2020-03-23 12:20 P J P
2020-03-23 12:20 ` [PATCH v6 1/2] net: tulip: check frame size and r/w data length P J P
2020-03-23 12:21 ` [PATCH v6 2/2] net: tulip: add .can_receive routine P J P
0 siblings, 2 replies; 14+ messages in thread
From: P J P @ 2020-03-23 12:20 UTC (permalink / raw)
To: Jason Wang
Cc: Prasad J Pandit, Stefan Hajnoczi, Sven Schnelle, Qemu Developers,
Li Qiang, Philippe Mathieu-Daudé,
Ziming Zhang
From: Prasad J Pandit <pjp@fedoraproject.org>
Hello,
* This series adds checks to avoid potential OOB access and infinite loop
issues while processing rx/tx data.
* Tulip tx descriptors are capped at 128 to avoid infinite loop in
tulip_xmit_list_update(), wrt Tulip kernel driver
-> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/dec/tulip/tulip.h#n319
* Update v3: add .can_receive routine
-> https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
* Update v4: flush queued packets once they are received
-> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg05868.html
* Update v5: fixed a typo in patch commit message
-> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06209.html
* Update v6: merge earlier patch 2 & 3 into one
-> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06868.html
Thank you.
--
Prasad J Pandit (2):
net: tulip: check frame size and r/w data length
net: tulip: add .can_receive routine
hw/net/tulip.c | 51 +++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 42 insertions(+), 9 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 1/2] net: tulip: check frame size and r/w data length
2020-03-23 12:20 [PATCH v6 0/2] net: tulip: add checks to avoid OOB access P J P
@ 2020-03-23 12:20 ` P J P
2020-03-24 1:29 ` Li Qiang
2020-03-23 12:21 ` [PATCH v6 2/2] net: tulip: add .can_receive routine P J P
1 sibling, 1 reply; 14+ messages in thread
From: P J P @ 2020-03-23 12:20 UTC (permalink / raw)
To: Jason Wang
Cc: Prasad J Pandit, Stefan Hajnoczi, Sven Schnelle, Qemu Developers,
Li Qiang, Philippe Mathieu-Daudé,
Ziming Zhang
From: Prasad J Pandit <pjp@fedoraproject.org>
Tulip network driver while copying tx/rx buffers does not check
frame size against r/w data length. This may lead to OOB buffer
access. Add check to avoid it.
Limit iterations over descriptors to avoid potential infinite
loop issue in tulip_xmit_list_update.
Reported-by: Li Qiang <pangpei.lq@antfin.com>
Reported-by: Ziming Zhang <ezrakiez@gmail.com>
Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
hw/net/tulip.c | 36 +++++++++++++++++++++++++++---------
1 file changed, 27 insertions(+), 9 deletions(-)
Update v3: return a value from tulip_copy_tx_buffers() and avoid infinite loop
-> https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index cfac2719d3..fbe40095da 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -170,6 +170,10 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct tulip_descriptor *desc)
} else {
len = s->rx_frame_len;
}
+
+ if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
+ return;
+ }
pci_dma_write(&s->dev, desc->buf_addr1, s->rx_frame +
(s->rx_frame_size - s->rx_frame_len), len);
s->rx_frame_len -= len;
@@ -181,6 +185,10 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct tulip_descriptor *desc)
} else {
len = s->rx_frame_len;
}
+
+ if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
+ return;
+ }
pci_dma_write(&s->dev, desc->buf_addr2, s->rx_frame +
(s->rx_frame_size - s->rx_frame_len), len);
s->rx_frame_len -= len;
@@ -227,7 +235,8 @@ static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf, size_t size)
trace_tulip_receive(buf, size);
- if (size < 14 || size > 2048 || s->rx_frame_len || tulip_rx_stopped(s)) {
+ if (size < 14 || size > sizeof(s->rx_frame) - 4
+ || s->rx_frame_len || tulip_rx_stopped(s)) {
return 0;
}
@@ -275,7 +284,6 @@ static ssize_t tulip_receive_nc(NetClientState *nc,
return tulip_receive(qemu_get_nic_opaque(nc), buf, size);
}
-
static NetClientInfo net_tulip_info = {
.type = NET_CLIENT_DRIVER_NIC,
.size = sizeof(NICState),
@@ -558,7 +566,7 @@ static void tulip_tx(TULIPState *s, struct tulip_descriptor *desc)
if ((s->csr[6] >> CSR6_OM_SHIFT) & CSR6_OM_MASK) {
/* Internal or external Loopback */
tulip_receive(s, s->tx_frame, s->tx_frame_len);
- } else {
+ } else if (s->tx_frame_len <= sizeof(s->tx_frame)) {
qemu_send_packet(qemu_get_queue(s->nic),
s->tx_frame, s->tx_frame_len);
}
@@ -570,23 +578,31 @@ static void tulip_tx(TULIPState *s, struct tulip_descriptor *desc)
}
}
-static void tulip_copy_tx_buffers(TULIPState *s, struct tulip_descriptor *desc)
+static int tulip_copy_tx_buffers(TULIPState *s, struct tulip_descriptor *desc)
{
int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT) & TDES1_BUF1_SIZE_MASK;
int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT) & TDES1_BUF2_SIZE_MASK;
+ if (s->tx_frame_len + len1 >= sizeof(s->tx_frame)) {
+ return -1;
+ }
if (len1) {
pci_dma_read(&s->dev, desc->buf_addr1,
s->tx_frame + s->tx_frame_len, len1);
s->tx_frame_len += len1;
}
+ if (s->tx_frame_len + len2 >= sizeof(s->tx_frame)) {
+ return -1;
+ }
if (len2) {
pci_dma_read(&s->dev, desc->buf_addr2,
s->tx_frame + s->tx_frame_len, len2);
s->tx_frame_len += len2;
}
desc->status = (len1 + len2) ? 0 : 0x7fffffff;
+
+ return 0;
}
static void tulip_setup_filter_addr(TULIPState *s, uint8_t *buf, int n)
@@ -651,13 +667,15 @@ static uint32_t tulip_ts(TULIPState *s)
static void tulip_xmit_list_update(TULIPState *s)
{
+#define TULIP_DESC_MAX 128
+ uint8_t i = 0;
struct tulip_descriptor desc;
if (tulip_ts(s) != CSR5_TS_SUSPENDED) {
return;
}
- for (;;) {
+ for (i = 0; i < TULIP_DESC_MAX; i++) {
tulip_desc_read(s, s->current_tx_desc, &desc);
tulip_dump_tx_descriptor(s, &desc);
@@ -675,10 +693,10 @@ static void tulip_xmit_list_update(TULIPState *s)
s->tx_frame_len = 0;
}
- tulip_copy_tx_buffers(s, &desc);
-
- if (desc.control & TDES1_LS) {
- tulip_tx(s, &desc);
+ if (!tulip_copy_tx_buffers(s, &desc)) {
+ if (desc.control & TDES1_LS) {
+ tulip_tx(s, &desc);
+ }
}
}
tulip_desc_write(s, s->current_tx_desc, &desc);
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v6 2/2] net: tulip: add .can_receive routine
2020-03-23 12:20 [PATCH v6 0/2] net: tulip: add checks to avoid OOB access P J P
2020-03-23 12:20 ` [PATCH v6 1/2] net: tulip: check frame size and r/w data length P J P
@ 2020-03-23 12:21 ` P J P
2020-03-24 2:04 ` Li Qiang
1 sibling, 1 reply; 14+ messages in thread
From: P J P @ 2020-03-23 12:21 UTC (permalink / raw)
To: Jason Wang
Cc: Prasad J Pandit, Stefan Hajnoczi, Sven Schnelle, Qemu Developers,
Li Qiang, Philippe Mathieu-Daudé,
Ziming Zhang
From: Prasad J Pandit <pjp@fedoraproject.org>
Define .can_receive routine to do sanity checks before receiving
packet data. And call qemu_flush_queued_packets to flush queued
packets once they are read in tulip_receive().
Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
hw/net/tulip.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
Update v6: merge earlier patch 2 & 3 into one
-> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06868.html
diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index fbe40095da..8d8c9519e7 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -229,6 +229,18 @@ static bool tulip_filter_address(TULIPState *s, const uint8_t *addr)
return ret;
}
+static int
+tulip_can_receive(NetClientState *nc)
+{
+ TULIPState *s = qemu_get_nic_opaque(nc);
+
+ if (s->rx_frame_len || tulip_rx_stopped(s)) {
+ return false;
+ }
+
+ return true;
+}
+
static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf, size_t size)
{
struct tulip_descriptor desc;
@@ -236,7 +248,7 @@ static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf, size_t size)
trace_tulip_receive(buf, size);
if (size < 14 || size > sizeof(s->rx_frame) - 4
- || s->rx_frame_len || tulip_rx_stopped(s)) {
+ || !tulip_can_receive(s->nic->ncs)) {
return 0;
}
@@ -275,6 +287,8 @@ static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf, size_t size)
tulip_desc_write(s, s->current_rx_desc, &desc);
tulip_next_rx_descriptor(s, &desc);
} while (s->rx_frame_len);
+
+ qemu_flush_queued_packets(qemu_get_queue(s->nic));
return size;
}
@@ -288,6 +302,7 @@ static NetClientInfo net_tulip_info = {
.type = NET_CLIENT_DRIVER_NIC,
.size = sizeof(NICState),
.receive = tulip_receive_nc,
+ .can_receive = tulip_can_receive,
};
static const char *tulip_reg_name(const hwaddr addr)
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
2020-03-23 12:20 ` [PATCH v6 1/2] net: tulip: check frame size and r/w data length P J P
@ 2020-03-24 1:29 ` Li Qiang
2020-03-24 5:45 ` Jason Wang
0 siblings, 1 reply; 14+ messages in thread
From: Li Qiang @ 2020-03-24 1:29 UTC (permalink / raw)
To: P J P
Cc: Prasad J Pandit, Stefan Hajnoczi, Jason Wang, Qemu Developers,
Philippe Mathieu-Daudé,
Li Qiang, Sven Schnelle, Ziming Zhang
[-- Attachment #1: Type: text/plain, Size: 5691 bytes --]
P J P <ppandit@redhat.com> 于2020年3月23日周一 下午8:24写道：
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Tulip network driver while copying tx/rx buffers does not check
> frame size against r/w data length. This may lead to OOB buffer
> access. Add check to avoid it.
>
> Limit iterations over descriptors to avoid potential infinite
> loop issue in tulip_xmit_list_update.
>
> Reported-by: Li Qiang <pangpei.lq@antfin.com>
> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> Reported-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>
Tested-by: Li Qiang <liq3ea@gmail.com>
But I have a minor question....
> ---
> hw/net/tulip.c | 36 +++++++++++++++++++++++++++---------
> 1 file changed, 27 insertions(+), 9 deletions(-)
>
> Update v3: return a value from tulip_copy_tx_buffers() and avoid infinite
> loop
> -> https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
>
> diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> index cfac2719d3..fbe40095da 100644
> --- a/hw/net/tulip.c
> +++ b/hw/net/tulip.c
> @@ -170,6 +170,10 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct
> tulip_descriptor *desc)
> } else {
> len = s->rx_frame_len;
> }
> +
> + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> + return;
> + }
>
Why here is '>=' instead of '>'.
IIUC the total sending length can reach to sizeof(s->rx_frame).
Same in the other place in this patch.
PS: I have planned to write a qtest case. But my personal qemu dev
environment is broken.
I will try to write it tonight or tomorrow night.
Thanks,
Li Qiang
> pci_dma_write(&s->dev, desc->buf_addr1, s->rx_frame +
> (s->rx_frame_size - s->rx_frame_len), len);
> s->rx_frame_len -= len;
> @@ -181,6 +185,10 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct
> tulip_descriptor *desc)
> } else {
> len = s->rx_frame_len;
> }
> +
> + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> + return;
> + }
> pci_dma_write(&s->dev, desc->buf_addr2, s->rx_frame +
> (s->rx_frame_size - s->rx_frame_len), len);
> s->rx_frame_len -= len;
> @@ -227,7 +235,8 @@ static ssize_t tulip_receive(TULIPState *s, const
> uint8_t *buf, size_t size)
>
> trace_tulip_receive(buf, size);
>
> - if (size < 14 || size > 2048 || s->rx_frame_len ||
> tulip_rx_stopped(s)) {
> + if (size < 14 || size > sizeof(s->rx_frame) - 4
> + || s->rx_frame_len || tulip_rx_stopped(s)) {
> return 0;
> }
>
> @@ -275,7 +284,6 @@ static ssize_t tulip_receive_nc(NetClientState *nc,
> return tulip_receive(qemu_get_nic_opaque(nc), buf, size);
> }
>
> -
> static NetClientInfo net_tulip_info = {
> .type = NET_CLIENT_DRIVER_NIC,
> .size = sizeof(NICState),
> @@ -558,7 +566,7 @@ static void tulip_tx(TULIPState *s, struct
> tulip_descriptor *desc)
> if ((s->csr[6] >> CSR6_OM_SHIFT) & CSR6_OM_MASK) {
> /* Internal or external Loopback */
> tulip_receive(s, s->tx_frame, s->tx_frame_len);
> - } else {
> + } else if (s->tx_frame_len <= sizeof(s->tx_frame)) {
> qemu_send_packet(qemu_get_queue(s->nic),
> s->tx_frame, s->tx_frame_len);
> }
> @@ -570,23 +578,31 @@ static void tulip_tx(TULIPState *s, struct
> tulip_descriptor *desc)
> }
> }
>
> -static void tulip_copy_tx_buffers(TULIPState *s, struct tulip_descriptor
> *desc)
> +static int tulip_copy_tx_buffers(TULIPState *s, struct tulip_descriptor
> *desc)
> {
> int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT) &
> TDES1_BUF1_SIZE_MASK;
> int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT) &
> TDES1_BUF2_SIZE_MASK;
>
> + if (s->tx_frame_len + len1 >= sizeof(s->tx_frame)) {
> + return -1;
> + }
> if (len1) {
> pci_dma_read(&s->dev, desc->buf_addr1,
> s->tx_frame + s->tx_frame_len, len1);
> s->tx_frame_len += len1;
> }
>
> + if (s->tx_frame_len + len2 >= sizeof(s->tx_frame)) {
> + return -1;
> + }
> if (len2) {
> pci_dma_read(&s->dev, desc->buf_addr2,
> s->tx_frame + s->tx_frame_len, len2);
> s->tx_frame_len += len2;
> }
> desc->status = (len1 + len2) ? 0 : 0x7fffffff;
> +
> + return 0;
> }
>
> static void tulip_setup_filter_addr(TULIPState *s, uint8_t *buf, int n)
> @@ -651,13 +667,15 @@ static uint32_t tulip_ts(TULIPState *s)
>
> static void tulip_xmit_list_update(TULIPState *s)
> {
> +#define TULIP_DESC_MAX 128
> + uint8_t i = 0;
> struct tulip_descriptor desc;
>
> if (tulip_ts(s) != CSR5_TS_SUSPENDED) {
> return;
> }
>
> - for (;;) {
> + for (i = 0; i < TULIP_DESC_MAX; i++) {
> tulip_desc_read(s, s->current_tx_desc, &desc);
> tulip_dump_tx_descriptor(s, &desc);
>
> @@ -675,10 +693,10 @@ static void tulip_xmit_list_update(TULIPState *s)
> s->tx_frame_len = 0;
> }
>
> - tulip_copy_tx_buffers(s, &desc);
> -
> - if (desc.control & TDES1_LS) {
> - tulip_tx(s, &desc);
> + if (!tulip_copy_tx_buffers(s, &desc)) {
> + if (desc.control & TDES1_LS) {
> + tulip_tx(s, &desc);
> + }
> }
> }
> tulip_desc_write(s, s->current_tx_desc, &desc);
> --
> 2.25.1
>
>
>
[-- Attachment #2: Type: text/html, Size: 7914 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] net: tulip: add .can_receive routine
2020-03-23 12:21 ` [PATCH v6 2/2] net: tulip: add .can_receive routine P J P
@ 2020-03-24 2:04 ` Li Qiang
2020-03-24 5:44 ` Jason Wang
0 siblings, 1 reply; 14+ messages in thread
From: Li Qiang @ 2020-03-24 2:04 UTC (permalink / raw)
To: P J P
Cc: Prasad J Pandit, Stefan Hajnoczi, Jason Wang, Qemu Developers,
Philippe Mathieu-Daudé,
Li Qiang, Sven Schnelle, Ziming Zhang
[-- Attachment #1: Type: text/plain, Size: 2461 bytes --]
P J P <ppandit@redhat.com> 于2020年3月23日周一 下午8:25写道：
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Define .can_receive routine to do sanity checks before receiving
> packet data. And call qemu_flush_queued_packets to flush queued
> packets once they are read in tulip_receive().
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/net/tulip.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> Update v6: merge earlier patch 2 & 3 into one
> -> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06868.html
>
> diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> index fbe40095da..8d8c9519e7 100644
> --- a/hw/net/tulip.c
> +++ b/hw/net/tulip.c
> @@ -229,6 +229,18 @@ static bool tulip_filter_address(TULIPState *s, const
> uint8_t *addr)
> return ret;
> }
>
> +static int
> +tulip_can_receive(NetClientState *nc)
> +{
> + TULIPState *s = qemu_get_nic_opaque(nc);
> +
> + if (s->rx_frame_len || tulip_rx_stopped(s)) {
> + return false;
> + }
> +
> + return true;
> +}
> +
> static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf, size_t
> size)
> {
> struct tulip_descriptor desc;
> @@ -236,7 +248,7 @@ static ssize_t tulip_receive(TULIPState *s, const
> uint8_t *buf, size_t size)
> trace_tulip_receive(buf, size);
>
> if (size < 14 || size > sizeof(s->rx_frame) - 4
> - || s->rx_frame_len || tulip_rx_stopped(s)) {
> + || !tulip_can_receive(s->nic->ncs)) {
> return 0;
> }
>
> @@ -275,6 +287,8 @@ static ssize_t tulip_receive(TULIPState *s, const
> uint8_t *buf, size_t size)
> tulip_desc_write(s, s->current_rx_desc, &desc);
> tulip_next_rx_descriptor(s, &desc);
> } while (s->rx_frame_len);
> +
> + qemu_flush_queued_packets(qemu_get_queue(s->nic));
>
Hi Prasad ans Jason,
I want to know why here we need call 'qemu_flush_queued_packets'.
AFAICS, the other networking card emulation need no this.
Thanks,
Li Qiang
> return size;
> }
>
> @@ -288,6 +302,7 @@ static NetClientInfo net_tulip_info = {
> .type = NET_CLIENT_DRIVER_NIC,
> .size = sizeof(NICState),
> .receive = tulip_receive_nc,
> + .can_receive = tulip_can_receive,
> };
>
> static const char *tulip_reg_name(const hwaddr addr)
> --
> 2.25.1
>
>
>
[-- Attachment #2: Type: text/html, Size: 3573 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] net: tulip: add .can_receive routine
2020-03-24 2:04 ` Li Qiang
@ 2020-03-24 5:44 ` Jason Wang
0 siblings, 0 replies; 14+ messages in thread
From: Jason Wang @ 2020-03-24 5:44 UTC (permalink / raw)
To: Li Qiang, P J P
Cc: Prasad J Pandit, Stefan Hajnoczi, Qemu Developers,
Philippe Mathieu-Daudé,
Li Qiang, Sven Schnelle, Ziming Zhang
On 2020/3/24 上午10:04, Li Qiang wrote:
>
>
> P J P <ppandit@redhat.com <mailto:ppandit@redhat.com>> 于2020年3月23日周一
> 下午8:25写道：
>
> From: Prasad J Pandit <pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org>>
>
> Define .can_receive routine to do sanity checks before receiving
> packet data. And call qemu_flush_queued_packets to flush queued
> packets once they are read in tulip_receive().
>
> Suggested-by: Jason Wang <jasowang@redhat.com
> <mailto:jasowang@redhat.com>>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org>>
> ---
> hw/net/tulip.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> Update v6: merge earlier patch 2 & 3 into one
> ->
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06868.html
>
> diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> index fbe40095da..8d8c9519e7 100644
> --- a/hw/net/tulip.c
> +++ b/hw/net/tulip.c
> @@ -229,6 +229,18 @@ static bool tulip_filter_address(TULIPState
> *s, const uint8_t *addr)
> return ret;
> }
>
> +static int
> +tulip_can_receive(NetClientState *nc)
> +{
> + TULIPState *s = qemu_get_nic_opaque(nc);
> +
> + if (s->rx_frame_len || tulip_rx_stopped(s)) {
> + return false;
> + }
> +
> + return true;
> +}
> +
> static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf,
> size_t size)
> {
> struct tulip_descriptor desc;
> @@ -236,7 +248,7 @@ static ssize_t tulip_receive(TULIPState *s,
> const uint8_t *buf, size_t size)
> trace_tulip_receive(buf, size);
>
> if (size < 14 || size > sizeof(s->rx_frame) - 4
> - || s->rx_frame_len || tulip_rx_stopped(s)) {
> + || !tulip_can_receive(s->nic->ncs)) {
> return 0;
> }
>
> @@ -275,6 +287,8 @@ static ssize_t tulip_receive(TULIPState *s,
> const uint8_t *buf, size_t size)
> tulip_desc_write(s, s->current_rx_desc, &desc);
> tulip_next_rx_descriptor(s, &desc);
> } while (s->rx_frame_len);
> +
> + qemu_flush_queued_packets(qemu_get_queue(s->nic));
>
>
>
> Hi Prasad ans Jason,
> I want to know why here we need call 'qemu_flush_queued_packets'.
> AFAICS, the other networking card emulation need no this.
Right, I thought we need this because rx_frame_len is checked in
tulip_can_receive().
But not I think it's unnecessary. According to kernel driver code, the
rx is not necessarily stopped when rx buffer is overrun, e.g,
tulip_rx_refill() only toggle the doorbell when chip is LC82C168. But
own emulation did DC21143.
This means using can_receive() is wrong.
So I think we can drop this patch.
Thanks for the checking and sorry for the wrong suggestion.
>
> Thanks,
> Li Qiang
>
> return size;
> }
>
> @@ -288,6 +302,7 @@ static NetClientInfo net_tulip_info = {
> .type = NET_CLIENT_DRIVER_NIC,
> .size = sizeof(NICState),
> .receive = tulip_receive_nc,
> + .can_receive = tulip_can_receive,
> };
>
> static const char *tulip_reg_name(const hwaddr addr)
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
2020-03-24 1:29 ` Li Qiang
@ 2020-03-24 5:45 ` Jason Wang
2020-03-24 13:19 ` P J P
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Jason Wang @ 2020-03-24 5:45 UTC (permalink / raw)
To: Li Qiang, P J P
Cc: Prasad J Pandit, Stefan Hajnoczi, Sven Schnelle, Qemu Developers,
Li Qiang, Philippe Mathieu-Daudé,
Ziming Zhang
On 2020/3/24 上午9:29, Li Qiang wrote:
>
>
> P J P <ppandit@redhat.com <mailto:ppandit@redhat.com>> 于2020年3月23日周一
> 下午8:24写道：
>
> From: Prasad J Pandit <pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org>>
>
> Tulip network driver while copying tx/rx buffers does not check
> frame size against r/w data length. This may lead to OOB buffer
> access. Add check to avoid it.
>
> Limit iterations over descriptors to avoid potential infinite
> loop issue in tulip_xmit_list_update.
>
> Reported-by: Li Qiang <pangpei.lq@antfin.com
> <mailto:pangpei.lq@antfin.com>>
> Reported-by: Ziming Zhang <ezrakiez@gmail.com
> <mailto:ezrakiez@gmail.com>>
> Reported-by: Jason Wang <jasowang@redhat.com
> <mailto:jasowang@redhat.com>>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org>>
>
>
>
> Tested-by: Li Qiang <liq3ea@gmail.com <mailto:liq3ea@gmail.com>>
> But I have a minor question....
>
> ---
> hw/net/tulip.c | 36 +++++++++++++++++++++++++++---------
> 1 file changed, 27 insertions(+), 9 deletions(-)
>
> Update v3: return a value from tulip_copy_tx_buffers() and avoid
> infinite loop
> ->
> https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
>
> diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> index cfac2719d3..fbe40095da 100644
> --- a/hw/net/tulip.c
> +++ b/hw/net/tulip.c
> @@ -170,6 +170,10 @@ static void tulip_copy_rx_bytes(TULIPState
> *s, struct tulip_descriptor *desc)
> } else {
> len = s->rx_frame_len;
> }
> +
> + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> + return;
> + }
>
>
>
> Why here is '>=' instead of '>'.
> IIUC the total sending length can reach to sizeof(s->rx_frame).
> Same in the other place in this patch.
Yes, this need to be fixed.
>
> PS: I have planned to write a qtest case. But my personal qemu dev
> environment is broken.
> I will try to write it tonight or tomorrow night.
Cool, good to know this.
Thanks
>
> Thanks,
> Li Qiang
>
>
>
>
> pci_dma_write(&s->dev, desc->buf_addr1, s->rx_frame +
> (s->rx_frame_size - s->rx_frame_len), len);
> s->rx_frame_len -= len;
> @@ -181,6 +185,10 @@ static void tulip_copy_rx_bytes(TULIPState
> *s, struct tulip_descriptor *desc)
> } else {
> len = s->rx_frame_len;
> }
> +
> + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> + return;
> + }
> pci_dma_write(&s->dev, desc->buf_addr2, s->rx_frame +
> (s->rx_frame_size - s->rx_frame_len), len);
> s->rx_frame_len -= len;
> @@ -227,7 +235,8 @@ static ssize_t tulip_receive(TULIPState *s,
> const uint8_t *buf, size_t size)
>
> trace_tulip_receive(buf, size);
>
> - if (size < 14 || size > 2048 || s->rx_frame_len ||
> tulip_rx_stopped(s)) {
> + if (size < 14 || size > sizeof(s->rx_frame) - 4
> + || s->rx_frame_len || tulip_rx_stopped(s)) {
> return 0;
> }
>
> @@ -275,7 +284,6 @@ static ssize_t tulip_receive_nc(NetClientState
> *nc,
> return tulip_receive(qemu_get_nic_opaque(nc), buf, size);
> }
>
> -
> static NetClientInfo net_tulip_info = {
> .type = NET_CLIENT_DRIVER_NIC,
> .size = sizeof(NICState),
> @@ -558,7 +566,7 @@ static void tulip_tx(TULIPState *s, struct
> tulip_descriptor *desc)
> if ((s->csr[6] >> CSR6_OM_SHIFT) & CSR6_OM_MASK) {
> /* Internal or external Loopback */
> tulip_receive(s, s->tx_frame, s->tx_frame_len);
> - } else {
> + } else if (s->tx_frame_len <= sizeof(s->tx_frame)) {
> qemu_send_packet(qemu_get_queue(s->nic),
> s->tx_frame, s->tx_frame_len);
> }
> @@ -570,23 +578,31 @@ static void tulip_tx(TULIPState *s, struct
> tulip_descriptor *desc)
> }
> }
>
> -static void tulip_copy_tx_buffers(TULIPState *s, struct
> tulip_descriptor *desc)
> +static int tulip_copy_tx_buffers(TULIPState *s, struct
> tulip_descriptor *desc)
> {
> int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT) &
> TDES1_BUF1_SIZE_MASK;
> int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT) &
> TDES1_BUF2_SIZE_MASK;
>
> + if (s->tx_frame_len + len1 >= sizeof(s->tx_frame)) {
> + return -1;
> + }
> if (len1) {
> pci_dma_read(&s->dev, desc->buf_addr1,
> s->tx_frame + s->tx_frame_len, len1);
> s->tx_frame_len += len1;
> }
>
> + if (s->tx_frame_len + len2 >= sizeof(s->tx_frame)) {
> + return -1;
> + }
> if (len2) {
> pci_dma_read(&s->dev, desc->buf_addr2,
> s->tx_frame + s->tx_frame_len, len2);
> s->tx_frame_len += len2;
> }
> desc->status = (len1 + len2) ? 0 : 0x7fffffff;
> +
> + return 0;
> }
>
> static void tulip_setup_filter_addr(TULIPState *s, uint8_t *buf,
> int n)
> @@ -651,13 +667,15 @@ static uint32_t tulip_ts(TULIPState *s)
>
> static void tulip_xmit_list_update(TULIPState *s)
> {
> +#define TULIP_DESC_MAX 128
> + uint8_t i = 0;
> struct tulip_descriptor desc;
>
> if (tulip_ts(s) != CSR5_TS_SUSPENDED) {
> return;
> }
>
> - for (;;) {
> + for (i = 0; i < TULIP_DESC_MAX; i++) {
> tulip_desc_read(s, s->current_tx_desc, &desc);
> tulip_dump_tx_descriptor(s, &desc);
>
> @@ -675,10 +693,10 @@ static void
> tulip_xmit_list_update(TULIPState *s)
> s->tx_frame_len = 0;
> }
>
> - tulip_copy_tx_buffers(s, &desc);
> -
> - if (desc.control & TDES1_LS) {
> - tulip_tx(s, &desc);
> + if (!tulip_copy_tx_buffers(s, &desc)) {
> + if (desc.control & TDES1_LS) {
> + tulip_tx(s, &desc);
> + }
> }
> }
> tulip_desc_write(s, s->current_tx_desc, &desc);
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
2020-03-24 5:45 ` Jason Wang
@ 2020-03-24 13:19 ` P J P
2020-03-24 14:54 ` Li Qiang
2020-03-26 4:34 ` P J P
2 siblings, 0 replies; 14+ messages in thread
From: P J P @ 2020-03-24 13:19 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, Li Qiang, Qemu Developers,
Philippe Mathieu-Daudé,
Li Qiang, Sven Schnelle, Ziming Zhang
[-- Attachment #1: Type: text/plain, Size: 535 bytes --]
+-- On Tue, 24 Mar 2020, Jason Wang wrote --+
| > + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
| > + return;
| > + }
| >
| > Why here is '>=' instead of '>'. IIUC the total sending length can reach
| > to sizeof(s->rx_frame). Same in the other place in this patch.
|
| Yes, this need to be fixed.
But, wouldn't s->rx_frame[sizeof(s->rx_frame)] be off-by-one?
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
2020-03-24 5:45 ` Jason Wang
2020-03-24 13:19 ` P J P
@ 2020-03-24 14:54 ` Li Qiang
2020-03-27 2:09 ` Jason Wang
2020-03-26 4:34 ` P J P
2 siblings, 1 reply; 14+ messages in thread
From: Li Qiang @ 2020-03-24 14:54 UTC (permalink / raw)
To: Jason Wang, Paolo Bonzini
Cc: Prasad J Pandit, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Qemu Developers, P J P, Li Qiang, Sven Schnelle, Ziming Zhang
[-- Attachment #1: Type: text/plain, Size: 9261 bytes --]
Jason Wang <jasowang@redhat.com> 于2020年3月24日周二 下午1:45写道：
>
> On 2020/3/24 上午9:29, Li Qiang wrote:
> >
> >
> > P J P <ppandit@redhat.com <mailto:ppandit@redhat.com>> 于2020年3月23日周一
> > 下午8:24写道：
> >
> > From: Prasad J Pandit <pjp@fedoraproject.org
> > <mailto:pjp@fedoraproject.org>>
> >
> > Tulip network driver while copying tx/rx buffers does not check
> > frame size against r/w data length. This may lead to OOB buffer
> > access. Add check to avoid it.
> >
> > Limit iterations over descriptors to avoid potential infinite
> > loop issue in tulip_xmit_list_update.
> >
> > Reported-by: Li Qiang <pangpei.lq@antfin.com
> > <mailto:pangpei.lq@antfin.com>>
> > Reported-by: Ziming Zhang <ezrakiez@gmail.com
> > <mailto:ezrakiez@gmail.com>>
> > Reported-by: Jason Wang <jasowang@redhat.com
> > <mailto:jasowang@redhat.com>>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org
> > <mailto:pjp@fedoraproject.org>>
> >
> >
> >
> > Tested-by: Li Qiang <liq3ea@gmail.com <mailto:liq3ea@gmail.com>>
> > But I have a minor question....
> >
> > ---
> > hw/net/tulip.c | 36 +++++++++++++++++++++++++++---------
> > 1 file changed, 27 insertions(+), 9 deletions(-)
> >
> > Update v3: return a value from tulip_copy_tx_buffers() and avoid
> > infinite loop
> > ->
> > https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
> >
> > diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> > index cfac2719d3..fbe40095da 100644
> > --- a/hw/net/tulip.c
> > +++ b/hw/net/tulip.c
> > @@ -170,6 +170,10 @@ static void tulip_copy_rx_bytes(TULIPState
> > *s, struct tulip_descriptor *desc)
> > } else {
> > len = s->rx_frame_len;
> > }
> > +
> > + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> > + return;
> > + }
> >
> >
> >
> > Why here is '>=' instead of '>'.
> > IIUC the total sending length can reach to sizeof(s->rx_frame).
> > Same in the other place in this patch.
>
>
> Yes, this need to be fixed.
>
>
> >
> > PS: I have planned to write a qtest case. But my personal qemu dev
> > environment is broken.
> > I will try to write it tonight or tomorrow night.
>
>
> Cool, good to know this.
>
>
Hi all,
I have countered an interesting issue. Let's look at the definition of
TULIPState.
21 typedef struct TULIPState {
22 PCIDevice dev;
23 MemoryRegion io;
24 MemoryRegion memory;
25 NICConf c;
26 qemu_irq irq;
27 NICState *nic;
28 eeprom_t *eeprom;
29 uint32_t csr[16];
30
31 /* state for MII */
32 uint32_t old_csr9;
33 uint32_t mii_word;
34 uint32_t mii_bitcnt;
35
36 hwaddr current_rx_desc;
37 hwaddr current_tx_desc;
38
39 uint8_t rx_frame[2048];
40 uint8_t tx_frame[2048];
41 uint16_t tx_frame_len;
42 uint16_t rx_frame_len;
43 uint16_t rx_frame_size;
44
45 uint32_t rx_status;
46 uint8_t filter[16][6];
47 } TULIPState;
Here we can see the overflow is occured after 'tx_frame'.
In my quest, I have see the overflow(the s->tx_frame_len is big).
However here doesn't cause SEGV in qtest.
In real case, the qemu process will access the data after TULIPState in
heap and trigger segv.
However in qtest mode I don't know how to trigger this.
The core code like this:
qpci_device_enable(dev);
bar = qpci_iomap(dev, 0, NULL);
context_pa = guest_alloc(alloc, sizeof(context));
guest_pa = guest_alloc(alloc, 4096);
memset(guest_data, 'A', sizeof(guest_data));
context[0].status = 1 << 31;
context[0].control = 0x7ff << 11 | 0x7ff;
context[0].buf_addr2 = context_pa + sizeof(struct tulip_descriptor);
context[0].buf_addr1 = guest_pa;
for (i = 1; i < ARRAY_SIZE(context); ++i) {
context_pa += sizeof(struct tulip_descriptor);
context[i].status = 1 << 31;
context[i].control = 0x7ff << 11 | 0x7ff;
context[i].buf_addr2 = context_pa + sizeof(struct tulip_descriptor);
context[i].buf_addr1 = guest_pa;
}
qtest_memwrite(dev->bus->qts, context_pa, context, sizeof(context));
qtest_memwrite(dev->bus->qts, guest_pa, guest_data, sizeof(guest_data));
qpci_io_writel(dev, bar, 0x20, context_pa);
qpci_io_writel(dev, bar, 0x30, 1 << 13);
Paolo may give some hints?
Thanks,
Li Qiang
> Thanks
>
>
> >
> > Thanks,
> > Li Qiang
> >
> >
> >
> >
> > pci_dma_write(&s->dev, desc->buf_addr1, s->rx_frame +
> > (s->rx_frame_size - s->rx_frame_len), len);
> > s->rx_frame_len -= len;
> > @@ -181,6 +185,10 @@ static void tulip_copy_rx_bytes(TULIPState
> > *s, struct tulip_descriptor *desc)
> > } else {
> > len = s->rx_frame_len;
> > }
> > +
> > + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> > + return;
> > + }
> > pci_dma_write(&s->dev, desc->buf_addr2, s->rx_frame +
> > (s->rx_frame_size - s->rx_frame_len), len);
> > s->rx_frame_len -= len;
> > @@ -227,7 +235,8 @@ static ssize_t tulip_receive(TULIPState *s,
> > const uint8_t *buf, size_t size)
> >
> > trace_tulip_receive(buf, size);
> >
> > - if (size < 14 || size > 2048 || s->rx_frame_len ||
> > tulip_rx_stopped(s)) {
> > + if (size < 14 || size > sizeof(s->rx_frame) - 4
> > + || s->rx_frame_len || tulip_rx_stopped(s)) {
> > return 0;
> > }
> >
> > @@ -275,7 +284,6 @@ static ssize_t tulip_receive_nc(NetClientState
> > *nc,
> > return tulip_receive(qemu_get_nic_opaque(nc), buf, size);
> > }
> >
> > -
> > static NetClientInfo net_tulip_info = {
> > .type = NET_CLIENT_DRIVER_NIC,
> > .size = sizeof(NICState),
> > @@ -558,7 +566,7 @@ static void tulip_tx(TULIPState *s, struct
> > tulip_descriptor *desc)
> > if ((s->csr[6] >> CSR6_OM_SHIFT) & CSR6_OM_MASK) {
> > /* Internal or external Loopback */
> > tulip_receive(s, s->tx_frame, s->tx_frame_len);
> > - } else {
> > + } else if (s->tx_frame_len <= sizeof(s->tx_frame)) {
> > qemu_send_packet(qemu_get_queue(s->nic),
> > s->tx_frame, s->tx_frame_len);
> > }
> > @@ -570,23 +578,31 @@ static void tulip_tx(TULIPState *s, struct
> > tulip_descriptor *desc)
> > }
> > }
> >
> > -static void tulip_copy_tx_buffers(TULIPState *s, struct
> > tulip_descriptor *desc)
> > +static int tulip_copy_tx_buffers(TULIPState *s, struct
> > tulip_descriptor *desc)
> > {
> > int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT) &
> > TDES1_BUF1_SIZE_MASK;
> > int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT) &
> > TDES1_BUF2_SIZE_MASK;
> >
> > + if (s->tx_frame_len + len1 >= sizeof(s->tx_frame)) {
> > + return -1;
> > + }
> > if (len1) {
> > pci_dma_read(&s->dev, desc->buf_addr1,
> > s->tx_frame + s->tx_frame_len, len1);
> > s->tx_frame_len += len1;
> > }
> >
> > + if (s->tx_frame_len + len2 >= sizeof(s->tx_frame)) {
> > + return -1;
> > + }
> > if (len2) {
> > pci_dma_read(&s->dev, desc->buf_addr2,
> > s->tx_frame + s->tx_frame_len, len2);
> > s->tx_frame_len += len2;
> > }
> > desc->status = (len1 + len2) ? 0 : 0x7fffffff;
> > +
> > + return 0;
> > }
> >
> > static void tulip_setup_filter_addr(TULIPState *s, uint8_t *buf,
> > int n)
> > @@ -651,13 +667,15 @@ static uint32_t tulip_ts(TULIPState *s)
> >
> > static void tulip_xmit_list_update(TULIPState *s)
> > {
> > +#define TULIP_DESC_MAX 128
> > + uint8_t i = 0;
> > struct tulip_descriptor desc;
> >
> > if (tulip_ts(s) != CSR5_TS_SUSPENDED) {
> > return;
> > }
> >
> > - for (;;) {
> > + for (i = 0; i < TULIP_DESC_MAX; i++) {
> > tulip_desc_read(s, s->current_tx_desc, &desc);
> > tulip_dump_tx_descriptor(s, &desc);
> >
> > @@ -675,10 +693,10 @@ static void
> > tulip_xmit_list_update(TULIPState *s)
> > s->tx_frame_len = 0;
> > }
> >
> > - tulip_copy_tx_buffers(s, &desc);
> > -
> > - if (desc.control & TDES1_LS) {
> > - tulip_tx(s, &desc);
> > + if (!tulip_copy_tx_buffers(s, &desc)) {
> > + if (desc.control & TDES1_LS) {
> > + tulip_tx(s, &desc);
> > + }
> > }
> > }
> > tulip_desc_write(s, s->current_tx_desc, &desc);
> > --
> > 2.25.1
> >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 13288 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
2020-03-24 5:45 ` Jason Wang
2020-03-24 13:19 ` P J P
2020-03-24 14:54 ` Li Qiang
@ 2020-03-26 4:34 ` P J P
2 siblings, 0 replies; 14+ messages in thread
From: P J P @ 2020-03-26 4:34 UTC (permalink / raw)
To: Jason Wang
Cc: Stefan Hajnoczi, Li Qiang, Qemu Developers,
Philippe Mathieu-Daudé,
Li Qiang, Sven Schnelle, Ziming Zhang
+-- On Tue, 24 Mar 2020, Jason Wang wrote --+
| > Why here is '>=' instead of '>'. IIUC the total sending length can reach
| > to sizeof(s->rx_frame). Same in the other place in this patch.
|
| Yes, this need to be fixed.
Sent patch v7. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
2020-03-24 14:54 ` Li Qiang
@ 2020-03-27 2:09 ` Jason Wang
2020-03-27 2:35 ` Li Qiang
0 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2020-03-27 2:09 UTC (permalink / raw)
To: Li Qiang, Paolo Bonzini
Cc: Prasad J Pandit, Stefan Hajnoczi, Sven Schnelle, Qemu Developers,
P J P, Li Qiang, Philippe Mathieu-Daudé,
Ziming Zhang
On 2020/3/24 下午10:54, Li Qiang wrote:
>
>
> Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
> 于2020年3月24日周二 下午1:45写道：
>
>
> On 2020/3/24 上午9:29, Li Qiang wrote:
> >
> >
> > P J P <ppandit@redhat.com <mailto:ppandit@redhat.com>
> <mailto:ppandit@redhat.com <mailto:ppandit@redhat.com>>>
> 于2020年3月23日周一
> > 下午8:24写道：
> >
> > From: Prasad J Pandit <pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org>
> > <mailto:pjp@fedoraproject.org <mailto:pjp@fedoraproject.org>>>
> >
> > Tulip network driver while copying tx/rx buffers does not check
> > frame size against r/w data length. This may lead to OOB buffer
> > access. Add check to avoid it.
> >
> > Limit iterations over descriptors to avoid potential infinite
> > loop issue in tulip_xmit_list_update.
> >
> > Reported-by: Li Qiang <pangpei.lq@antfin.com
> <mailto:pangpei.lq@antfin.com>
> > <mailto:pangpei.lq@antfin.com <mailto:pangpei.lq@antfin.com>>>
> > Reported-by: Ziming Zhang <ezrakiez@gmail.com
> <mailto:ezrakiez@gmail.com>
> > <mailto:ezrakiez@gmail.com <mailto:ezrakiez@gmail.com>>>
> > Reported-by: Jason Wang <jasowang@redhat.com
> <mailto:jasowang@redhat.com>
> > <mailto:jasowang@redhat.com <mailto:jasowang@redhat.com>>>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org>
> > <mailto:pjp@fedoraproject.org <mailto:pjp@fedoraproject.org>>>
> >
> >
> >
> > Tested-by: Li Qiang <liq3ea@gmail.com <mailto:liq3ea@gmail.com>
> <mailto:liq3ea@gmail.com <mailto:liq3ea@gmail.com>>>
> > But I have a minor question....
> >
> > ---
> > hw/net/tulip.c | 36 +++++++++++++++++++++++++++---------
> > 1 file changed, 27 insertions(+), 9 deletions(-)
> >
> > Update v3: return a value from tulip_copy_tx_buffers() and avoid
> > infinite loop
> > ->
> > https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
> >
> > diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> > index cfac2719d3..fbe40095da 100644
> > --- a/hw/net/tulip.c
> > +++ b/hw/net/tulip.c
> > @@ -170,6 +170,10 @@ static void tulip_copy_rx_bytes(TULIPState
> > *s, struct tulip_descriptor *desc)
> > } else {
> > len = s->rx_frame_len;
> > }
> > +
> > + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> > + return;
> > + }
> >
> >
> >
> > Why here is '>=' instead of '>'.
> > IIUC the total sending length can reach to sizeof(s->rx_frame).
> > Same in the other place in this patch.
>
>
> Yes, this need to be fixed.
>
>
> >
> > PS: I have planned to write a qtest case. But my personal qemu dev
> > environment is broken.
> > I will try to write it tonight or tomorrow night.
>
>
> Cool, good to know this.
>
>
> Hi all,
> I have countered an interesting issue. Let's look at the definition of
> TULIPState.
>
> 21 typedef struct TULIPState {
> 22 PCIDevice dev;
> 23 MemoryRegion io;
> 24 MemoryRegion memory;
> 25 NICConf c;
> 26 qemu_irq irq;
> 27 NICState *nic;
> 28 eeprom_t *eeprom;
> 29 uint32_t csr[16];
> 30
> 31 /* state for MII */
> 32 uint32_t old_csr9;
> 33 uint32_t mii_word;
> 34 uint32_t mii_bitcnt;
> 35
> 36 hwaddr current_rx_desc;
> 37 hwaddr current_tx_desc;
> 38
> 39 uint8_t rx_frame[2048];
> 40 uint8_t tx_frame[2048];
> 41 uint16_t tx_frame_len;
> 42 uint16_t rx_frame_len;
> 43 uint16_t rx_frame_size;
> 44
> 45 uint32_t rx_status;
> 46 uint8_t filter[16][6];
> 47 } TULIPState;
>
> Here we can see the overflow is occured after 'tx_frame'.
> In my quest, I have see the overflow(the s->tx_frame_len is big).
> However here doesn't cause SEGV in qtest.
> In real case, the qemu process will access the data after TULIPState
> in heap and trigger segv.
> However in qtest mode I don't know how to trigger this.
If it's just the mangling of tx_frame_len, it won't hit SIGSEV.
I wonder maybe, somehow that large tx_frame_len is used for buffer
copying or other stuffs that can lead the crash.
Thanks
>
> The core code like this:
>
> qpci_device_enable(dev);
> bar = qpci_iomap(dev, 0, NULL);
> context_pa = guest_alloc(alloc, sizeof(context));
> guest_pa = guest_alloc(alloc, 4096);
> memset(guest_data, 'A', sizeof(guest_data));
> context[0].status = 1 << 31;
> context[0].control = 0x7ff << 11 | 0x7ff;
> context[0].buf_addr2 = context_pa + sizeof(struct tulip_descriptor);
> context[0].buf_addr1 = guest_pa;
> for (i = 1; i < ARRAY_SIZE(context); ++i) {
> context_pa += sizeof(struct tulip_descriptor);
> context[i].status = 1 << 31;
> context[i].control = 0x7ff << 11 | 0x7ff;
> context[i].buf_addr2 = context_pa + sizeof(struct tulip_descriptor);
> context[i].buf_addr1 = guest_pa;
> }
>
> qtest_memwrite(dev->bus->qts, context_pa, context, sizeof(context));
> qtest_memwrite(dev->bus->qts, guest_pa, guest_data, sizeof(guest_data));
> qpci_io_writel(dev, bar, 0x20, context_pa);
> qpci_io_writel(dev, bar, 0x30, 1 << 13);
>
> Paolo may give some hints?
>
> Thanks,
> Li Qiang
>
> Thanks
>
>
> >
> > Thanks,
> > Li Qiang
> >
> >
> >
> >
> > pci_dma_write(&s->dev, desc->buf_addr1, s->rx_frame +
> > (s->rx_frame_size - s->rx_frame_len), len);
> > s->rx_frame_len -= len;
> > @@ -181,6 +185,10 @@ static void tulip_copy_rx_bytes(TULIPState
> > *s, struct tulip_descriptor *desc)
> > } else {
> > len = s->rx_frame_len;
> > }
> > +
> > + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> > + return;
> > + }
> > pci_dma_write(&s->dev, desc->buf_addr2, s->rx_frame +
> > (s->rx_frame_size - s->rx_frame_len), len);
> > s->rx_frame_len -= len;
> > @@ -227,7 +235,8 @@ static ssize_t tulip_receive(TULIPState *s,
> > const uint8_t *buf, size_t size)
> >
> > trace_tulip_receive(buf, size);
> >
> > - if (size < 14 || size > 2048 || s->rx_frame_len ||
> > tulip_rx_stopped(s)) {
> > + if (size < 14 || size > sizeof(s->rx_frame) - 4
> > + || s->rx_frame_len || tulip_rx_stopped(s)) {
> > return 0;
> > }
> >
> > @@ -275,7 +284,6 @@ static ssize_t
> tulip_receive_nc(NetClientState
> > *nc,
> > return tulip_receive(qemu_get_nic_opaque(nc), buf, size);
> > }
> >
> > -
> > static NetClientInfo net_tulip_info = {
> > .type = NET_CLIENT_DRIVER_NIC,
> > .size = sizeof(NICState),
> > @@ -558,7 +566,7 @@ static void tulip_tx(TULIPState *s, struct
> > tulip_descriptor *desc)
> > if ((s->csr[6] >> CSR6_OM_SHIFT) & CSR6_OM_MASK) {
> > /* Internal or external Loopback */
> > tulip_receive(s, s->tx_frame, s->tx_frame_len);
> > - } else {
> > + } else if (s->tx_frame_len <= sizeof(s->tx_frame)) {
> > qemu_send_packet(qemu_get_queue(s->nic),
> > s->tx_frame, s->tx_frame_len);
> > }
> > @@ -570,23 +578,31 @@ static void tulip_tx(TULIPState *s, struct
> > tulip_descriptor *desc)
> > }
> > }
> >
> > -static void tulip_copy_tx_buffers(TULIPState *s, struct
> > tulip_descriptor *desc)
> > +static int tulip_copy_tx_buffers(TULIPState *s, struct
> > tulip_descriptor *desc)
> > {
> > int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT) &
> > TDES1_BUF1_SIZE_MASK;
> > int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT) &
> > TDES1_BUF2_SIZE_MASK;
> >
> > + if (s->tx_frame_len + len1 >= sizeof(s->tx_frame)) {
> > + return -1;
> > + }
> > if (len1) {
> > pci_dma_read(&s->dev, desc->buf_addr1,
> > s->tx_frame + s->tx_frame_len, len1);
> > s->tx_frame_len += len1;
> > }
> >
> > + if (s->tx_frame_len + len2 >= sizeof(s->tx_frame)) {
> > + return -1;
> > + }
> > if (len2) {
> > pci_dma_read(&s->dev, desc->buf_addr2,
> > s->tx_frame + s->tx_frame_len, len2);
> > s->tx_frame_len += len2;
> > }
> > desc->status = (len1 + len2) ? 0 : 0x7fffffff;
> > +
> > + return 0;
> > }
> >
> > static void tulip_setup_filter_addr(TULIPState *s, uint8_t
> *buf,
> > int n)
> > @@ -651,13 +667,15 @@ static uint32_t tulip_ts(TULIPState *s)
> >
> > static void tulip_xmit_list_update(TULIPState *s)
> > {
> > +#define TULIP_DESC_MAX 128
> > + uint8_t i = 0;
> > struct tulip_descriptor desc;
> >
> > if (tulip_ts(s) != CSR5_TS_SUSPENDED) {
> > return;
> > }
> >
> > - for (;;) {
> > + for (i = 0; i < TULIP_DESC_MAX; i++) {
> > tulip_desc_read(s, s->current_tx_desc, &desc);
> > tulip_dump_tx_descriptor(s, &desc);
> >
> > @@ -675,10 +693,10 @@ static void
> > tulip_xmit_list_update(TULIPState *s)
> > s->tx_frame_len = 0;
> > }
> >
> > - tulip_copy_tx_buffers(s, &desc);
> > -
> > - if (desc.control & TDES1_LS) {
> > - tulip_tx(s, &desc);
> > + if (!tulip_copy_tx_buffers(s, &desc)) {
> > + if (desc.control & TDES1_LS) {
> > + tulip_tx(s, &desc);
> > + }
> > }
> > }
> > tulip_desc_write(s, s->current_tx_desc, &desc);
> > --
> > 2.25.1
> >
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
2020-03-27 2:09 ` Jason Wang
@ 2020-03-27 2:35 ` Li Qiang
2020-03-27 2:53 ` Jason Wang
0 siblings, 1 reply; 14+ messages in thread
From: Li Qiang @ 2020-03-27 2:35 UTC (permalink / raw)
To: Jason Wang
Cc: Prasad J Pandit, Stefan Hajnoczi, Sven Schnelle, Qemu Developers,
P J P, Li Qiang, Paolo Bonzini, Philippe Mathieu-Daudé,
Ziming Zhang
[-- Attachment #1: Type: text/plain, Size: 14453 bytes --]
Jason Wang <jasowang@redhat.com> 于2020年3月27日周五 上午10:09写道：
>
> On 2020/3/24 下午10:54, Li Qiang wrote:
> >
> >
> > Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
> > 于2020年3月24日周二 下午1:45写道：
> >
> >
> > On 2020/3/24 上午9:29, Li Qiang wrote:
> > >
> > >
> > > P J P <ppandit@redhat.com <mailto:ppandit@redhat.com>
> > <mailto:ppandit@redhat.com <mailto:ppandit@redhat.com>>>
> > 于2020年3月23日周一
> > > 下午8:24写道：
> > >
> > > From: Prasad J Pandit <pjp@fedoraproject.org
> > <mailto:pjp@fedoraproject.org>
> > > <mailto:pjp@fedoraproject.org <mailto:pjp@fedoraproject.org>>>
> > >
> > > Tulip network driver while copying tx/rx buffers does not check
> > > frame size against r/w data length. This may lead to OOB buffer
> > > access. Add check to avoid it.
> > >
> > > Limit iterations over descriptors to avoid potential infinite
> > > loop issue in tulip_xmit_list_update.
> > >
> > > Reported-by: Li Qiang <pangpei.lq@antfin.com
> > <mailto:pangpei.lq@antfin.com>
> > > <mailto:pangpei.lq@antfin.com <mailto:pangpei.lq@antfin.com>>>
> > > Reported-by: Ziming Zhang <ezrakiez@gmail.com
> > <mailto:ezrakiez@gmail.com>
> > > <mailto:ezrakiez@gmail.com <mailto:ezrakiez@gmail.com>>>
> > > Reported-by: Jason Wang <jasowang@redhat.com
> > <mailto:jasowang@redhat.com>
> > > <mailto:jasowang@redhat.com <mailto:jasowang@redhat.com>>>
> > > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org
> > <mailto:pjp@fedoraproject.org>
> > > <mailto:pjp@fedoraproject.org <mailto:pjp@fedoraproject.org>>>
> > >
> > >
> > >
> > > Tested-by: Li Qiang <liq3ea@gmail.com <mailto:liq3ea@gmail.com>
> > <mailto:liq3ea@gmail.com <mailto:liq3ea@gmail.com>>>
> > > But I have a minor question....
> > >
> > > ---
> > > hw/net/tulip.c | 36 +++++++++++++++++++++++++++---------
> > > 1 file changed, 27 insertions(+), 9 deletions(-)
> > >
> > > Update v3: return a value from tulip_copy_tx_buffers() and
> avoid
> > > infinite loop
> > > ->
> > >
> https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
> > >
> > > diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> > > index cfac2719d3..fbe40095da 100644
> > > --- a/hw/net/tulip.c
> > > +++ b/hw/net/tulip.c
> > > @@ -170,6 +170,10 @@ static void tulip_copy_rx_bytes(TULIPState
> > > *s, struct tulip_descriptor *desc)
> > > } else {
> > > len = s->rx_frame_len;
> > > }
> > > +
> > > + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> > > + return;
> > > + }
> > >
> > >
> > >
> > > Why here is '>=' instead of '>'.
> > > IIUC the total sending length can reach to sizeof(s->rx_frame).
> > > Same in the other place in this patch.
> >
> >
> > Yes, this need to be fixed.
> >
> >
> > >
> > > PS: I have planned to write a qtest case. But my personal qemu dev
> > > environment is broken.
> > > I will try to write it tonight or tomorrow night.
> >
> >
> > Cool, good to know this.
> >
> >
> > Hi all,
> > I have countered an interesting issue. Let's look at the definition of
> > TULIPState.
> >
> > 21 typedef struct TULIPState {
> > 22 PCIDevice dev;
> > 23 MemoryRegion io;
> > 24 MemoryRegion memory;
> > 25 NICConf c;
> > 26 qemu_irq irq;
> > 27 NICState *nic;
> > 28 eeprom_t *eeprom;
> > 29 uint32_t csr[16];
> > 30
> > 31 /* state for MII */
> > 32 uint32_t old_csr9;
> > 33 uint32_t mii_word;
> > 34 uint32_t mii_bitcnt;
> > 35
> > 36 hwaddr current_rx_desc;
> > 37 hwaddr current_tx_desc;
> > 38
> > 39 uint8_t rx_frame[2048];
> > 40 uint8_t tx_frame[2048];
> > 41 uint16_t tx_frame_len;
> > 42 uint16_t rx_frame_len;
> > 43 uint16_t rx_frame_size;
> > 44
> > 45 uint32_t rx_status;
> > 46 uint8_t filter[16][6];
> > 47 } TULIPState;
> >
> > Here we can see the overflow is occured after 'tx_frame'.
> > In my quest, I have see the overflow(the s->tx_frame_len is big).
> > However here doesn't cause SEGV in qtest.
> > In real case, the qemu process will access the data after TULIPState
> > in heap and trigger segv.
> > However in qtest mode I don't know how to trigger this.
>
>
> If it's just the mangling of tx_frame_len, it won't hit SIGSEV.
>
> I wonder maybe, somehow that large tx_frame_len is used for buffer
> copying or other stuffs that can lead the crash.
>
This is because in real qemu process, the OOB copy corrupts the head data
after 'TULIPState' struct.
And maybe later(other thread) access the corrupted data thus leading crash.
However in qtest mode, I don't remember the core code of qtest. But seems
it's not a really VM? just a interface emulation.
In my case, it's backtrace is as this:
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffbdb7fe700 (LWP 60813)]
0x0000000000000000 in ?? ()
...
(gdb) bt
#0 0x0000000000000000 in ()
#1 0x0000555555a7dfa3 in qemu_set_irq (irq=0x5555579e0710, level=1) at
hw/core/irq.c:44
#2 0x0000555555b2b87a in tulip_update_int (s=0x5555579da7c0) at
hw/net/tulip.c:121
#3 0x0000555555b2cdd9 in tulip_xmit_list_update (s=0x5555579da7c0) at
hw/net/tulip.c:667
#4 0x0000555555b2d19d in tulip_write (opaque=0x5555579da7c0, addr=32,
data=931909632, size=4) at hw/net/tulip.c:759
#5 0x000055555587eed1 in memory_region_write_accessor (mr=0x5555579db0b0,
addr=32, value=0x7ffbdb7fd6a8, size=4, shift=0, mask=4294967295, attrs=...)
at /xxx/qemu/memory.c:483
#6 0x000055555587f0da in access_with_adjusted_size (addr=32,
value=0x7ffbdb7fd6a8, size=4, access_size_min=4, access_size_max=4,
access_fn=0x55555587edf2 <memory_region_write_accessor>, mr=0x5555579db0b0,
attrs=...)
at /xxx/qemu/memory.c:544
#7 0x000055555588213b in memory_region_dispatch_write (mr=0x5555579db0b0,
addr=32, data=931909632, op=MO_32, attrs=...) at /xxx/qemu/memory.c:1476
#8 0x000055555581fe9c in flatview_write_continue (fv=0x7ffbb001eae0,
addr=49184, attrs=..., ptr=0x7ffff7e13000, len=4, addr1=32, l=4,
mr=0x5555579db0b0) at /xxx/qemu/exec.c:3125
#9 0x000055555581fff4 in flatview_write (fv=0x7ffbb001eae0, addr=49184,
attrs=..., buf=0x7ffff7e13000, len=4) at /xxx/qemu/exec.c:3165
#10 0x0000555555820368 in address_space_write (as=0x555556762560
<address_space_io>, addr=49184, attrs=..., buf=0x7ffff7e13000, len=4) at
/xxx/qemu/exec.c:3256
#11 0x00005555558203da in address_space_rw (as=0x555556762560
<address_space_io>, addr=49184, attrs=..., buf=0x7ffff7e13000, len=4,
is_write=true) at /xxx/qemu/exec.c:3266
#12 0x000055555589723b in kvm_handle_io (port=49184, attrs=...,
data=0x7ffff7e13000, direction=1, size=4, count=1) at
/xxx/qemu/accel/kvm/kvm-all.c:2140
#13 0x00005555558979d6 in kvm_cpu_exec (cpu=0x555556b1e220) at
/xxx/qemu/accel/kvm/kvm-all.c:2386
#14 0x00005555558701c5 in qemu_kvm_cpu_thread_fn (arg=0x555556b1e220) at
/xxx/qemu/cpus.c:1246
#15 0x0000555555de3ce4 in qemu_thread_start (args=0x555556b44040) at
util/qemu-thread-posix.c:519
#16 0x00007ffff5bb0e25 in start_thread () at /lib64/libpthread.so.0
#17 0x00007ffff58d8f1d in clone () at /lib64/libc.so.6
I will try to dig into the qtest code and hope find a way to trigger a segv
in qtest.
Thanks,
Li Qiang
>
> Thanks
>
>
> >
> > The core code like this:
> >
> > qpci_device_enable(dev);
> > bar = qpci_iomap(dev, 0, NULL);
> > context_pa = guest_alloc(alloc, sizeof(context));
> > guest_pa = guest_alloc(alloc, 4096);
> > memset(guest_data, 'A', sizeof(guest_data));
> > context[0].status = 1 << 31;
> > context[0].control = 0x7ff << 11 | 0x7ff;
> > context[0].buf_addr2 = context_pa + sizeof(struct tulip_descriptor);
> > context[0].buf_addr1 = guest_pa;
> > for (i = 1; i < ARRAY_SIZE(context); ++i) {
> > context_pa += sizeof(struct tulip_descriptor);
> > context[i].status = 1 << 31;
> > context[i].control = 0x7ff << 11 | 0x7ff;
> > context[i].buf_addr2 = context_pa + sizeof(struct tulip_descriptor);
> > context[i].buf_addr1 = guest_pa;
> > }
> >
> > qtest_memwrite(dev->bus->qts, context_pa, context, sizeof(context));
> > qtest_memwrite(dev->bus->qts, guest_pa, guest_data, sizeof(guest_data));
> > qpci_io_writel(dev, bar, 0x20, context_pa);
> > qpci_io_writel(dev, bar, 0x30, 1 << 13);
> >
> > Paolo may give some hints?
> >
> > Thanks,
> > Li Qiang
> >
> > Thanks
> >
> >
> > >
> > > Thanks,
> > > Li Qiang
> > >
> > >
> > >
> > >
> > > pci_dma_write(&s->dev, desc->buf_addr1, s->rx_frame +
> > > (s->rx_frame_size - s->rx_frame_len), len);
> > > s->rx_frame_len -= len;
> > > @@ -181,6 +185,10 @@ static void tulip_copy_rx_bytes(TULIPState
> > > *s, struct tulip_descriptor *desc)
> > > } else {
> > > len = s->rx_frame_len;
> > > }
> > > +
> > > + if (s->rx_frame_len + len >= sizeof(s->rx_frame)) {
> > > + return;
> > > + }
> > > pci_dma_write(&s->dev, desc->buf_addr2, s->rx_frame +
> > > (s->rx_frame_size - s->rx_frame_len), len);
> > > s->rx_frame_len -= len;
> > > @@ -227,7 +235,8 @@ static ssize_t tulip_receive(TULIPState *s,
> > > const uint8_t *buf, size_t size)
> > >
> > > trace_tulip_receive(buf, size);
> > >
> > > - if (size < 14 || size > 2048 || s->rx_frame_len ||
> > > tulip_rx_stopped(s)) {
> > > + if (size < 14 || size > sizeof(s->rx_frame) - 4
> > > + || s->rx_frame_len || tulip_rx_stopped(s)) {
> > > return 0;
> > > }
> > >
> > > @@ -275,7 +284,6 @@ static ssize_t
> > tulip_receive_nc(NetClientState
> > > *nc,
> > > return tulip_receive(qemu_get_nic_opaque(nc), buf, size);
> > > }
> > >
> > > -
> > > static NetClientInfo net_tulip_info = {
> > > .type = NET_CLIENT_DRIVER_NIC,
> > > .size = sizeof(NICState),
> > > @@ -558,7 +566,7 @@ static void tulip_tx(TULIPState *s, struct
> > > tulip_descriptor *desc)
> > > if ((s->csr[6] >> CSR6_OM_SHIFT) & CSR6_OM_MASK) {
> > > /* Internal or external Loopback */
> > > tulip_receive(s, s->tx_frame, s->tx_frame_len);
> > > - } else {
> > > + } else if (s->tx_frame_len <= sizeof(s->tx_frame)) {
> > > qemu_send_packet(qemu_get_queue(s->nic),
> > > s->tx_frame, s->tx_frame_len);
> > > }
> > > @@ -570,23 +578,31 @@ static void tulip_tx(TULIPState *s,
> struct
> > > tulip_descriptor *desc)
> > > }
> > > }
> > >
> > > -static void tulip_copy_tx_buffers(TULIPState *s, struct
> > > tulip_descriptor *desc)
> > > +static int tulip_copy_tx_buffers(TULIPState *s, struct
> > > tulip_descriptor *desc)
> > > {
> > > int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT) &
> > > TDES1_BUF1_SIZE_MASK;
> > > int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT) &
> > > TDES1_BUF2_SIZE_MASK;
> > >
> > > + if (s->tx_frame_len + len1 >= sizeof(s->tx_frame)) {
> > > + return -1;
> > > + }
> > > if (len1) {
> > > pci_dma_read(&s->dev, desc->buf_addr1,
> > > s->tx_frame + s->tx_frame_len, len1);
> > > s->tx_frame_len += len1;
> > > }
> > >
> > > + if (s->tx_frame_len + len2 >= sizeof(s->tx_frame)) {
> > > + return -1;
> > > + }
> > > if (len2) {
> > > pci_dma_read(&s->dev, desc->buf_addr2,
> > > s->tx_frame + s->tx_frame_len, len2);
> > > s->tx_frame_len += len2;
> > > }
> > > desc->status = (len1 + len2) ? 0 : 0x7fffffff;
> > > +
> > > + return 0;
> > > }
> > >
> > > static void tulip_setup_filter_addr(TULIPState *s, uint8_t
> > *buf,
> > > int n)
> > > @@ -651,13 +667,15 @@ static uint32_t tulip_ts(TULIPState *s)
> > >
> > > static void tulip_xmit_list_update(TULIPState *s)
> > > {
> > > +#define TULIP_DESC_MAX 128
> > > + uint8_t i = 0;
> > > struct tulip_descriptor desc;
> > >
> > > if (tulip_ts(s) != CSR5_TS_SUSPENDED) {
> > > return;
> > > }
> > >
> > > - for (;;) {
> > > + for (i = 0; i < TULIP_DESC_MAX; i++) {
> > > tulip_desc_read(s, s->current_tx_desc, &desc);
> > > tulip_dump_tx_descriptor(s, &desc);
> > >
> > > @@ -675,10 +693,10 @@ static void
> > > tulip_xmit_list_update(TULIPState *s)
> > > s->tx_frame_len = 0;
> > > }
> > >
> > > - tulip_copy_tx_buffers(s, &desc);
> > > -
> > > - if (desc.control & TDES1_LS) {
> > > - tulip_tx(s, &desc);
> > > + if (!tulip_copy_tx_buffers(s, &desc)) {
> > > + if (desc.control & TDES1_LS) {
> > > + tulip_tx(s, &desc);
> > > + }
> > > }
> > > }
> > > tulip_desc_write(s, s->current_tx_desc, &desc);
> > > --
> > > 2.25.1
> > >
> > >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 21024 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
2020-03-27 2:35 ` Li Qiang
@ 2020-03-27 2:53 ` Jason Wang
2020-03-27 3:43 ` Li Qiang
0 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2020-03-27 2:53 UTC (permalink / raw)
To: Li Qiang
Cc: Prasad J Pandit, Stefan Hajnoczi, Sven Schnelle, Qemu Developers,
P J P, Li Qiang, Paolo Bonzini, Philippe Mathieu-Daudé,
Ziming Zhang
On 2020/3/27 上午10:35, Li Qiang wrote:
>
>
> Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
> 于2020年3月27日周五 上午10:09写道：
>
>
> On 2020/3/24 下午10:54, Li Qiang wrote:
> >
> >
> > Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>
> <mailto:jasowang@redhat.com <mailto:jasowang@redhat.com>>>
> > 于2020年3月24日周二 下午1:45写道：
> >
> >
> > On 2020/3/24 上午9:29, Li Qiang wrote:
> > >
> > >
> > > P J P <ppandit@redhat.com <mailto:ppandit@redhat.com>
> <mailto:ppandit@redhat.com <mailto:ppandit@redhat.com>>
> > <mailto:ppandit@redhat.com <mailto:ppandit@redhat.com>
> <mailto:ppandit@redhat.com <mailto:ppandit@redhat.com>>>>
> > 于2020年3月23日周一
> > > 下午8:24写道：
> > >
> > > From: Prasad J Pandit <pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org>
> > <mailto:pjp@fedoraproject.org <mailto:pjp@fedoraproject.org>>
> > > <mailto:pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org> <mailto:pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org>>>>
> > >
> > > Tulip network driver while copying tx/rx buffers does
> not check
> > > frame size against r/w data length. This may lead to
> OOB buffer
> > > access. Add check to avoid it.
> > >
> > > Limit iterations over descriptors to avoid potential
> infinite
> > > loop issue in tulip_xmit_list_update.
> > >
> > > Reported-by: Li Qiang <pangpei.lq@antfin.com
> <mailto:pangpei.lq@antfin.com>
> > <mailto:pangpei.lq@antfin.com <mailto:pangpei.lq@antfin.com>>
> > > <mailto:pangpei.lq@antfin.com
> <mailto:pangpei.lq@antfin.com> <mailto:pangpei.lq@antfin.com
> <mailto:pangpei.lq@antfin.com>>>>
> > > Reported-by: Ziming Zhang <ezrakiez@gmail.com
> <mailto:ezrakiez@gmail.com>
> > <mailto:ezrakiez@gmail.com <mailto:ezrakiez@gmail.com>>
> > > <mailto:ezrakiez@gmail.com <mailto:ezrakiez@gmail.com>
> <mailto:ezrakiez@gmail.com <mailto:ezrakiez@gmail.com>>>>
> > > Reported-by: Jason Wang <jasowang@redhat.com
> <mailto:jasowang@redhat.com>
> > <mailto:jasowang@redhat.com <mailto:jasowang@redhat.com>>
> > > <mailto:jasowang@redhat.com
> <mailto:jasowang@redhat.com> <mailto:jasowang@redhat.com
> <mailto:jasowang@redhat.com>>>>
> > > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org>
> > <mailto:pjp@fedoraproject.org <mailto:pjp@fedoraproject.org>>
> > > <mailto:pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org> <mailto:pjp@fedoraproject.org
> <mailto:pjp@fedoraproject.org>>>>
> > >
> > >
> > >
> > > Tested-by: Li Qiang <liq3ea@gmail.com
> <mailto:liq3ea@gmail.com> <mailto:liq3ea@gmail.com
> <mailto:liq3ea@gmail.com>>
> > <mailto:liq3ea@gmail.com <mailto:liq3ea@gmail.com>
> <mailto:liq3ea@gmail.com <mailto:liq3ea@gmail.com>>>>
> > > But I have a minor question....
> > >
> > > ---
> > > hw/net/tulip.c | 36 +++++++++++++++++++++++++++---------
> > > 1 file changed, 27 insertions(+), 9 deletions(-)
> > >
> > > Update v3: return a value from tulip_copy_tx_buffers()
> and avoid
> > > infinite loop
> > > ->
> > >
> https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
> > >
> > > diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> > > index cfac2719d3..fbe40095da 100644
> > > --- a/hw/net/tulip.c
> > > +++ b/hw/net/tulip.c
> > > @@ -170,6 +170,10 @@ static void
> tulip_copy_rx_bytes(TULIPState
> > > *s, struct tulip_descriptor *desc)
> > > } else {
> > > len = s->rx_frame_len;
> > > }
> > > +
> > > + if (s->rx_frame_len + len >=
> sizeof(s->rx_frame)) {
> > > + return;
> > > + }
> > >
> > >
> > >
> > > Why here is '>=' instead of '>'.
> > > IIUC the total sending length can reach to
> sizeof(s->rx_frame).
> > > Same in the other place in this patch.
> >
> >
> > Yes, this need to be fixed.
> >
> >
> > >
> > > PS: I have planned to write a qtest case. But my personal
> qemu dev
> > > environment is broken.
> > > I will try to write it tonight or tomorrow night.
> >
> >
> > Cool, good to know this.
> >
> >
> > Hi all,
> > I have countered an interesting issue. Let's look at the
> definition of
> > TULIPState.
> >
> > 21 typedef struct TULIPState {
> > 22 PCIDevice dev;
> > 23 MemoryRegion io;
> > 24 MemoryRegion memory;
> > 25 NICConf c;
> > 26 qemu_irq irq;
> > 27 NICState *nic;
> > 28 eeprom_t *eeprom;
> > 29 uint32_t csr[16];
> > 30
> > 31 /* state for MII */
> > 32 uint32_t old_csr9;
> > 33 uint32_t mii_word;
> > 34 uint32_t mii_bitcnt;
> > 35
> > 36 hwaddr current_rx_desc;
> > 37 hwaddr current_tx_desc;
> > 38
> > 39 uint8_t rx_frame[2048];
> > 40 uint8_t tx_frame[2048];
> > 41 uint16_t tx_frame_len;
> > 42 uint16_t rx_frame_len;
> > 43 uint16_t rx_frame_size;
> > 44
> > 45 uint32_t rx_status;
> > 46 uint8_t filter[16][6];
> > 47 } TULIPState;
> >
> > Here we can see the overflow is occured after 'tx_frame'.
> > In my quest, I have see the overflow(the s->tx_frame_len is big).
> > However here doesn't cause SEGV in qtest.
> > In real case, the qemu process will access the data after
> TULIPState
> > in heap and trigger segv.
> > However in qtest mode I don't know how to trigger this.
>
>
> If it's just the mangling of tx_frame_len, it won't hit SIGSEV.
>
> I wonder maybe, somehow that large tx_frame_len is used for buffer
> copying or other stuffs that can lead the crash.
>
>
> This is because in real qemu process, the OOB copy corrupts the head
> data after 'TULIPState' struct.
> And maybe later(other thread) access the corrupted data thus leading
> crash.
Ok, so I think ASAN can detect this crash. But I'm not sure whether or
not it was enabled for qtest. If not, we probably need that.
I wrote a qtest for virtio-net that can lead OOB, so it should probably
work for tulip but need to check.
Or if you don't want to depend on ASAN, we can check user visible status
after tx_frame[], but it looks to me all other fields are not visible by
guest.
Maybe we can reorder to place csr[] after tx_frame[] then check csr[]
afterwards.
> However in qtest mode, I don't remember the core code of qtest. But
> seems it's not a really VM? just a interface emulation.
If my memory is correct, it's not a VM.
Thanks
> In my case, it's backtrace is as this:
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7ffbdb7fe700 (LWP 60813)]
> 0x0000000000000000 in ?? ()
> ...
> (gdb) bt
> #0 0x0000000000000000 in ()
> #1 0x0000555555a7dfa3 in qemu_set_irq (irq=0x5555579e0710, level=1)
> at hw/core/irq.c:44
> #2 0x0000555555b2b87a in tulip_update_int (s=0x5555579da7c0) at
> hw/net/tulip.c:121
> #3 0x0000555555b2cdd9 in tulip_xmit_list_update (s=0x5555579da7c0) at
> hw/net/tulip.c:667
> #4 0x0000555555b2d19d in tulip_write (opaque=0x5555579da7c0, addr=32,
> data=931909632, size=4) at hw/net/tulip.c:759
> #5 0x000055555587eed1 in memory_region_write_accessor
> (mr=0x5555579db0b0, addr=32, value=0x7ffbdb7fd6a8, size=4, shift=0,
> mask=4294967295, attrs=...) at /xxx/qemu/memory.c:483
> #6 0x000055555587f0da in access_with_adjusted_size (addr=32,
> value=0x7ffbdb7fd6a8, size=4, access_size_min=4, access_size_max=4,
> access_fn=0x55555587edf2 <memory_region_write_accessor>,
> mr=0x5555579db0b0, attrs=...)
> at /xxx/qemu/memory.c:544
> #7 0x000055555588213b in memory_region_dispatch_write
> (mr=0x5555579db0b0, addr=32, data=931909632, op=MO_32, attrs=...) at
> /xxx/qemu/memory.c:1476
> #8 0x000055555581fe9c in flatview_write_continue (fv=0x7ffbb001eae0,
> addr=49184, attrs=..., ptr=0x7ffff7e13000, len=4, addr1=32, l=4,
> mr=0x5555579db0b0) at /xxx/qemu/exec.c:3125
> #9 0x000055555581fff4 in flatview_write (fv=0x7ffbb001eae0,
> addr=49184, attrs=..., buf=0x7ffff7e13000, len=4) at /xxx/qemu/exec.c:3165
> #10 0x0000555555820368 in address_space_write (as=0x555556762560
> <address_space_io>, addr=49184, attrs=..., buf=0x7ffff7e13000, len=4)
> at /xxx/qemu/exec.c:3256
> #11 0x00005555558203da in address_space_rw (as=0x555556762560
> <address_space_io>, addr=49184, attrs=..., buf=0x7ffff7e13000, len=4,
> is_write=true) at /xxx/qemu/exec.c:3266
> #12 0x000055555589723b in kvm_handle_io (port=49184, attrs=...,
> data=0x7ffff7e13000, direction=1, size=4, count=1) at
> /xxx/qemu/accel/kvm/kvm-all.c:2140
> #13 0x00005555558979d6 in kvm_cpu_exec (cpu=0x555556b1e220) at
> /xxx/qemu/accel/kvm/kvm-all.c:2386
> #14 0x00005555558701c5 in qemu_kvm_cpu_thread_fn (arg=0x555556b1e220)
> at /xxx/qemu/cpus.c:1246
> #15 0x0000555555de3ce4 in qemu_thread_start (args=0x555556b44040) at
> util/qemu-thread-posix.c:519
> #16 0x00007ffff5bb0e25 in start_thread () at /lib64/libpthread.so.0
> #17 0x00007ffff58d8f1d in clone () at /lib64/libc.so.6
>
> I will try to dig into the qtest code and hope find a way to trigger a
> segv in qtest.
>
> Thanks,
> Li Qiang
>
>
> Thanks
>
>
> >
> > The core code like this:
> >
> > qpci_device_enable(dev);
> > bar = qpci_iomap(dev, 0, NULL);
> > context_pa = guest_alloc(alloc, sizeof(context));
> > guest_pa = guest_alloc(alloc, 4096);
> > memset(guest_data, 'A', sizeof(guest_data));
> > context[0].status = 1 << 31;
> > context[0].control = 0x7ff << 11 | 0x7ff;
> > context[0].buf_addr2 = context_pa + sizeof(struct tulip_descriptor);
> > context[0].buf_addr1 = guest_pa;
> > for (i = 1; i < ARRAY_SIZE(context); ++i) {
> > context_pa += sizeof(struct tulip_descriptor);
> > context[i].status = 1 << 31;
> > context[i].control = 0x7ff << 11 | 0x7ff;
> > context[i].buf_addr2 = context_pa + sizeof(struct tulip_descriptor);
> > context[i].buf_addr1 = guest_pa;
> > }
> >
> > qtest_memwrite(dev->bus->qts, context_pa, context, sizeof(context));
> > qtest_memwrite(dev->bus->qts, guest_pa, guest_data,
> sizeof(guest_data));
> > qpci_io_writel(dev, bar, 0x20, context_pa);
> > qpci_io_writel(dev, bar, 0x30, 1 << 13);
> >
> > Paolo may give some hints?
> >
> > Thanks,
> > Li Qiang
> >
> > Thanks
> >
> >
> > >
> > > Thanks,
> > > Li Qiang
> > >
> > >
> > >
> > >
> > > pci_dma_write(&s->dev, desc->buf_addr1,
> s->rx_frame +
> > > (s->rx_frame_size - s->rx_frame_len), len);
> > > s->rx_frame_len -= len;
> > > @@ -181,6 +185,10 @@ static void
> tulip_copy_rx_bytes(TULIPState
> > > *s, struct tulip_descriptor *desc)
> > > } else {
> > > len = s->rx_frame_len;
> > > }
> > > +
> > > + if (s->rx_frame_len + len >=
> sizeof(s->rx_frame)) {
> > > + return;
> > > + }
> > > pci_dma_write(&s->dev, desc->buf_addr2,
> s->rx_frame +
> > > (s->rx_frame_size - s->rx_frame_len), len);
> > > s->rx_frame_len -= len;
> > > @@ -227,7 +235,8 @@ static ssize_t
> tulip_receive(TULIPState *s,
> > > const uint8_t *buf, size_t size)
> > >
> > > trace_tulip_receive(buf, size);
> > >
> > > - if (size < 14 || size > 2048 || s->rx_frame_len ||
> > > tulip_rx_stopped(s)) {
> > > + if (size < 14 || size > sizeof(s->rx_frame) - 4
> > > + || s->rx_frame_len || tulip_rx_stopped(s)) {
> > > return 0;
> > > }
> > >
> > > @@ -275,7 +284,6 @@ static ssize_t
> > tulip_receive_nc(NetClientState
> > > *nc,
> > > return tulip_receive(qemu_get_nic_opaque(nc),
> buf, size);
> > > }
> > >
> > > -
> > > static NetClientInfo net_tulip_info = {
> > > .type = NET_CLIENT_DRIVER_NIC,
> > > .size = sizeof(NICState),
> > > @@ -558,7 +566,7 @@ static void tulip_tx(TULIPState
> *s, struct
> > > tulip_descriptor *desc)
> > > if ((s->csr[6] >> CSR6_OM_SHIFT) &
> CSR6_OM_MASK) {
> > > /* Internal or external Loopback */
> > > tulip_receive(s, s->tx_frame,
> s->tx_frame_len);
> > > - } else {
> > > + } else if (s->tx_frame_len <=
> sizeof(s->tx_frame)) {
> > > qemu_send_packet(qemu_get_queue(s->nic),
> > > s->tx_frame, s->tx_frame_len);
> > > }
> > > @@ -570,23 +578,31 @@ static void tulip_tx(TULIPState
> *s, struct
> > > tulip_descriptor *desc)
> > > }
> > > }
> > >
> > > -static void tulip_copy_tx_buffers(TULIPState *s, struct
> > > tulip_descriptor *desc)
> > > +static int tulip_copy_tx_buffers(TULIPState *s, struct
> > > tulip_descriptor *desc)
> > > {
> > > int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT) &
> > > TDES1_BUF1_SIZE_MASK;
> > > int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT) &
> > > TDES1_BUF2_SIZE_MASK;
> > >
> > > + if (s->tx_frame_len + len1 >= sizeof(s->tx_frame)) {
> > > + return -1;
> > > + }
> > > if (len1) {
> > > pci_dma_read(&s->dev, desc->buf_addr1,
> > > s->tx_frame + s->tx_frame_len, len1);
> > > s->tx_frame_len += len1;
> > > }
> > >
> > > + if (s->tx_frame_len + len2 >= sizeof(s->tx_frame)) {
> > > + return -1;
> > > + }
> > > if (len2) {
> > > pci_dma_read(&s->dev, desc->buf_addr2,
> > > s->tx_frame + s->tx_frame_len, len2);
> > > s->tx_frame_len += len2;
> > > }
> > > desc->status = (len1 + len2) ? 0 : 0x7fffffff;
> > > +
> > > + return 0;
> > > }
> > >
> > > static void tulip_setup_filter_addr(TULIPState *s,
> uint8_t
> > *buf,
> > > int n)
> > > @@ -651,13 +667,15 @@ static uint32_t
> tulip_ts(TULIPState *s)
> > >
> > > static void tulip_xmit_list_update(TULIPState *s)
> > > {
> > > +#define TULIP_DESC_MAX 128
> > > + uint8_t i = 0;
> > > struct tulip_descriptor desc;
> > >
> > > if (tulip_ts(s) != CSR5_TS_SUSPENDED) {
> > > return;
> > > }
> > >
> > > - for (;;) {
> > > + for (i = 0; i < TULIP_DESC_MAX; i++) {
> > > tulip_desc_read(s, s->current_tx_desc, &desc);
> > > tulip_dump_tx_descriptor(s, &desc);
> > >
> > > @@ -675,10 +693,10 @@ static void
> > > tulip_xmit_list_update(TULIPState *s)
> > > s->tx_frame_len = 0;
> > > }
> > >
> > > - tulip_copy_tx_buffers(s, &desc);
> > > -
> > > - if (desc.control & TDES1_LS) {
> > > - tulip_tx(s, &desc);
> > > + if (!tulip_copy_tx_buffers(s, &desc)) {
> > > + if (desc.control & TDES1_LS) {
> > > + tulip_tx(s, &desc);
> > > + }
> > > }
> > > }
> > > tulip_desc_write(s, s->current_tx_desc, &desc);
> > > --
> > > 2.25.1
> > >
> > >
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/2] net: tulip: check frame size and r/w data length
2020-03-27 2:53 ` Jason Wang
@ 2020-03-27 3:43 ` Li Qiang
0 siblings, 0 replies; 14+ messages in thread
From: Li Qiang @ 2020-03-27 3:43 UTC (permalink / raw)
To: Jason Wang
Cc: Prasad J Pandit, Stefan Hajnoczi, Sven Schnelle, Qemu Developers,
P J P, Li Qiang, Paolo Bonzini, Philippe Mathieu-Daudé,
Ziming Zhang
[-- Attachment #1: Type: text/plain, Size: 18803 bytes --]
Jason Wang <jasowang@redhat.com> 于2020年3月27日周五 上午10:53写道：
>
> On 2020/3/27 上午10:35, Li Qiang wrote:
> >
> >
> > Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
> > 于2020年3月27日周五 上午10:09写道：
> >
> >
> > On 2020/3/24 下午10:54, Li Qiang wrote:
> > >
> > >
> > > Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>
> > <mailto:jasowang@redhat.com <mailto:jasowang@redhat.com>>>
> > > 于2020年3月24日周二 下午1:45写道：
> > >
> > >
> > > On 2020/3/24 上午9:29, Li Qiang wrote:
> > > >
> > > >
> > > > P J P <ppandit@redhat.com <mailto:ppandit@redhat.com>
> > <mailto:ppandit@redhat.com <mailto:ppandit@redhat.com>>
> > > <mailto:ppandit@redhat.com <mailto:ppandit@redhat.com>
> > <mailto:ppandit@redhat.com <mailto:ppandit@redhat.com>>>>
> > > 于2020年3月23日周一
> > > > 下午8:24写道：
> > > >
> > > > From: Prasad J Pandit <pjp@fedoraproject.org
> > <mailto:pjp@fedoraproject.org>
> > > <mailto:pjp@fedoraproject.org <mailto:pjp@fedoraproject.org>>
> > > > <mailto:pjp@fedoraproject.org
> > <mailto:pjp@fedoraproject.org> <mailto:pjp@fedoraproject.org
> > <mailto:pjp@fedoraproject.org>>>>
> > > >
> > > > Tulip network driver while copying tx/rx buffers does
> > not check
> > > > frame size against r/w data length. This may lead to
> > OOB buffer
> > > > access. Add check to avoid it.
> > > >
> > > > Limit iterations over descriptors to avoid potential
> > infinite
> > > > loop issue in tulip_xmit_list_update.
> > > >
> > > > Reported-by: Li Qiang <pangpei.lq@antfin.com
> > <mailto:pangpei.lq@antfin.com>
> > > <mailto:pangpei.lq@antfin.com <mailto:pangpei.lq@antfin.com>>
> > > > <mailto:pangpei.lq@antfin.com
> > <mailto:pangpei.lq@antfin.com> <mailto:pangpei.lq@antfin.com
> > <mailto:pangpei.lq@antfin.com>>>>
> > > > Reported-by: Ziming Zhang <ezrakiez@gmail.com
> > <mailto:ezrakiez@gmail.com>
> > > <mailto:ezrakiez@gmail.com <mailto:ezrakiez@gmail.com>>
> > > > <mailto:ezrakiez@gmail.com <mailto:ezrakiez@gmail.com>
> > <mailto:ezrakiez@gmail.com <mailto:ezrakiez@gmail.com>>>>
> > > > Reported-by: Jason Wang <jasowang@redhat.com
> > <mailto:jasowang@redhat.com>
> > > <mailto:jasowang@redhat.com <mailto:jasowang@redhat.com>>
> > > > <mailto:jasowang@redhat.com
> > <mailto:jasowang@redhat.com> <mailto:jasowang@redhat.com
> > <mailto:jasowang@redhat.com>>>>
> > > > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org
> > <mailto:pjp@fedoraproject.org>
> > > <mailto:pjp@fedoraproject.org <mailto:pjp@fedoraproject.org>>
> > > > <mailto:pjp@fedoraproject.org
> > <mailto:pjp@fedoraproject.org> <mailto:pjp@fedoraproject.org
> > <mailto:pjp@fedoraproject.org>>>>
> > > >
> > > >
> > > >
> > > > Tested-by: Li Qiang <liq3ea@gmail.com
> > <mailto:liq3ea@gmail.com> <mailto:liq3ea@gmail.com
> > <mailto:liq3ea@gmail.com>>
> > > <mailto:liq3ea@gmail.com <mailto:liq3ea@gmail.com>
> > <mailto:liq3ea@gmail.com <mailto:liq3ea@gmail.com>>>>
> > > > But I have a minor question....
> > > >
> > > > ---
> > > > hw/net/tulip.c | 36 +++++++++++++++++++++++++++---------
> > > > 1 file changed, 27 insertions(+), 9 deletions(-)
> > > >
> > > > Update v3: return a value from tulip_copy_tx_buffers()
> > and avoid
> > > > infinite loop
> > > > ->
> > > >
> > https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg06275.html
> > > >
> > > > diff --git a/hw/net/tulip.c b/hw/net/tulip.c
> > > > index cfac2719d3..fbe40095da 100644
> > > > --- a/hw/net/tulip.c
> > > > +++ b/hw/net/tulip.c
> > > > @@ -170,6 +170,10 @@ static void
> > tulip_copy_rx_bytes(TULIPState
> > > > *s, struct tulip_descriptor *desc)
> > > > } else {
> > > > len = s->rx_frame_len;
> > > > }
> > > > +
> > > > + if (s->rx_frame_len + len >=
> > sizeof(s->rx_frame)) {
> > > > + return;
> > > > + }
> > > >
> > > >
> > > >
> > > > Why here is '>=' instead of '>'.
> > > > IIUC the total sending length can reach to
> > sizeof(s->rx_frame).
> > > > Same in the other place in this patch.
> > >
> > >
> > > Yes, this need to be fixed.
> > >
> > >
> > > >
> > > > PS: I have planned to write a qtest case. But my personal
> > qemu dev
> > > > environment is broken.
> > > > I will try to write it tonight or tomorrow night.
> > >
> > >
> > > Cool, good to know this.
> > >
> > >
> > > Hi all,
> > > I have countered an interesting issue. Let's look at the
> > definition of
> > > TULIPState.
> > >
> > > 21 typedef struct TULIPState {
> > > 22 PCIDevice dev;
> > > 23 MemoryRegion io;
> > > 24 MemoryRegion memory;
> > > 25 NICConf c;
> > > 26 qemu_irq irq;
> > > 27 NICState *nic;
> > > 28 eeprom_t *eeprom;
> > > 29 uint32_t csr[16];
> > > 30
> > > 31 /* state for MII */
> > > 32 uint32_t old_csr9;
> > > 33 uint32_t mii_word;
> > > 34 uint32_t mii_bitcnt;
> > > 35
> > > 36 hwaddr current_rx_desc;
> > > 37 hwaddr current_tx_desc;
> > > 38
> > > 39 uint8_t rx_frame[2048];
> > > 40 uint8_t tx_frame[2048];
> > > 41 uint16_t tx_frame_len;
> > > 42 uint16_t rx_frame_len;
> > > 43 uint16_t rx_frame_size;
> > > 44
> > > 45 uint32_t rx_status;
> > > 46 uint8_t filter[16][6];
> > > 47 } TULIPState;
> > >
> > > Here we can see the overflow is occured after 'tx_frame'.
> > > In my quest, I have see the overflow(the s->tx_frame_len is big).
> > > However here doesn't cause SEGV in qtest.
> > > In real case, the qemu process will access the data after
> > TULIPState
> > > in heap and trigger segv.
> > > However in qtest mode I don't know how to trigger this.
> >
> >
> > If it's just the mangling of tx_frame_len, it won't hit SIGSEV.
> >
> > I wonder maybe, somehow that large tx_frame_len is used for buffer
> > copying or other stuffs that can lead the crash.
> >
> >
> > This is because in real qemu process, the OOB copy corrupts the head
> > data after 'TULIPState' struct.
> > And maybe later(other thread) access the corrupted data thus leading
> > crash.
>
>
> Ok, so I think ASAN can detect this crash. But I'm not sure whether or
> not it was enabled for qtest. If not, we probably need that.
>
>
Yes, I think this is the solution.
> I wrote a qtest for virtio-net that can lead OOB, so it should probably
> work for tulip but need to check.
>
> Or if you don't want to depend on ASAN, we can check user visible status
> after tx_frame[], but it looks to me all other fields are not visible by
> guest.
>
>
Right, I have spent a lot of time to find a guest visible status but not
find it.
> Maybe we can reorder to place csr[] after tx_frame[] then check csr[]
> afterwards.
>
>
I think it's not worth to change this just for a test case.
I will test the ASAN solution asap.
Thanks,
Li Qiang
>
> > However in qtest mode, I don't remember the core code of qtest. But
> > seems it's not a really VM? just a interface emulation.
>
>
> If my memory is correct, it's not a VM.
>
> Thanks
>
>
> > In my case, it's backtrace is as this:
> > Program received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7ffbdb7fe700 (LWP 60813)]
> > 0x0000000000000000 in ?? ()
> > ...
> > (gdb) bt
> > #0 0x0000000000000000 in ()
> > #1 0x0000555555a7dfa3 in qemu_set_irq (irq=0x5555579e0710, level=1)
> > at hw/core/irq.c:44
> > #2 0x0000555555b2b87a in tulip_update_int (s=0x5555579da7c0) at
> > hw/net/tulip.c:121
> > #3 0x0000555555b2cdd9 in tulip_xmit_list_update (s=0x5555579da7c0) at
> > hw/net/tulip.c:667
> > #4 0x0000555555b2d19d in tulip_write (opaque=0x5555579da7c0, addr=32,
> > data=931909632, size=4) at hw/net/tulip.c:759
> > #5 0x000055555587eed1 in memory_region_write_accessor
> > (mr=0x5555579db0b0, addr=32, value=0x7ffbdb7fd6a8, size=4, shift=0,
> > mask=4294967295, attrs=...) at /xxx/qemu/memory.c:483
> > #6 0x000055555587f0da in access_with_adjusted_size (addr=32,
> > value=0x7ffbdb7fd6a8, size=4, access_size_min=4, access_size_max=4,
> > access_fn=0x55555587edf2 <memory_region_write_accessor>,
> > mr=0x5555579db0b0, attrs=...)
> > at /xxx/qemu/memory.c:544
> > #7 0x000055555588213b in memory_region_dispatch_write
> > (mr=0x5555579db0b0, addr=32, data=931909632, op=MO_32, attrs=...) at
> > /xxx/qemu/memory.c:1476
> > #8 0x000055555581fe9c in flatview_write_continue (fv=0x7ffbb001eae0,
> > addr=49184, attrs=..., ptr=0x7ffff7e13000, len=4, addr1=32, l=4,
> > mr=0x5555579db0b0) at /xxx/qemu/exec.c:3125
> > #9 0x000055555581fff4 in flatview_write (fv=0x7ffbb001eae0,
> > addr=49184, attrs=..., buf=0x7ffff7e13000, len=4) at
> /xxx/qemu/exec.c:3165
> > #10 0x0000555555820368 in address_space_write (as=0x555556762560
> > <address_space_io>, addr=49184, attrs=..., buf=0x7ffff7e13000, len=4)
> > at /xxx/qemu/exec.c:3256
> > #11 0x00005555558203da in address_space_rw (as=0x555556762560
> > <address_space_io>, addr=49184, attrs=..., buf=0x7ffff7e13000, len=4,
> > is_write=true) at /xxx/qemu/exec.c:3266
> > #12 0x000055555589723b in kvm_handle_io (port=49184, attrs=...,
> > data=0x7ffff7e13000, direction=1, size=4, count=1) at
> > /xxx/qemu/accel/kvm/kvm-all.c:2140
> > #13 0x00005555558979d6 in kvm_cpu_exec (cpu=0x555556b1e220) at
> > /xxx/qemu/accel/kvm/kvm-all.c:2386
> > #14 0x00005555558701c5 in qemu_kvm_cpu_thread_fn (arg=0x555556b1e220)
> > at /xxx/qemu/cpus.c:1246
> > #15 0x0000555555de3ce4 in qemu_thread_start (args=0x555556b44040) at
> > util/qemu-thread-posix.c:519
> > #16 0x00007ffff5bb0e25 in start_thread () at /lib64/libpthread.so.0
> > #17 0x00007ffff58d8f1d in clone () at /lib64/libc.so.6
> >
> > I will try to dig into the qtest code and hope find a way to trigger a
> > segv in qtest.
> >
> > Thanks,
> > Li Qiang
> >
> >
> > Thanks
> >
> >
> > >
> > > The core code like this:
> > >
> > > qpci_device_enable(dev);
> > > bar = qpci_iomap(dev, 0, NULL);
> > > context_pa = guest_alloc(alloc, sizeof(context));
> > > guest_pa = guest_alloc(alloc, 4096);
> > > memset(guest_data, 'A', sizeof(guest_data));
> > > context[0].status = 1 << 31;
> > > context[0].control = 0x7ff << 11 | 0x7ff;
> > > context[0].buf_addr2 = context_pa + sizeof(struct
> tulip_descriptor);
> > > context[0].buf_addr1 = guest_pa;
> > > for (i = 1; i < ARRAY_SIZE(context); ++i) {
> > > context_pa += sizeof(struct tulip_descriptor);
> > > context[i].status = 1 << 31;
> > > context[i].control = 0x7ff << 11 | 0x7ff;
> > > context[i].buf_addr2 = context_pa + sizeof(struct
> tulip_descriptor);
> > > context[i].buf_addr1 = guest_pa;
> > > }
> > >
> > > qtest_memwrite(dev->bus->qts, context_pa, context,
> sizeof(context));
> > > qtest_memwrite(dev->bus->qts, guest_pa, guest_data,
> > sizeof(guest_data));
> > > qpci_io_writel(dev, bar, 0x20, context_pa);
> > > qpci_io_writel(dev, bar, 0x30, 1 << 13);
> > >
> > > Paolo may give some hints?
> > >
> > > Thanks,
> > > Li Qiang
> > >
> > > Thanks
> > >
> > >
> > > >
> > > > Thanks,
> > > > Li Qiang
> > > >
> > > >
> > > >
> > > >
> > > > pci_dma_write(&s->dev, desc->buf_addr1,
> > s->rx_frame +
> > > > (s->rx_frame_size - s->rx_frame_len), len);
> > > > s->rx_frame_len -= len;
> > > > @@ -181,6 +185,10 @@ static void
> > tulip_copy_rx_bytes(TULIPState
> > > > *s, struct tulip_descriptor *desc)
> > > > } else {
> > > > len = s->rx_frame_len;
> > > > }
> > > > +
> > > > + if (s->rx_frame_len + len >=
> > sizeof(s->rx_frame)) {
> > > > + return;
> > > > + }
> > > > pci_dma_write(&s->dev, desc->buf_addr2,
> > s->rx_frame +
> > > > (s->rx_frame_size - s->rx_frame_len), len);
> > > > s->rx_frame_len -= len;
> > > > @@ -227,7 +235,8 @@ static ssize_t
> > tulip_receive(TULIPState *s,
> > > > const uint8_t *buf, size_t size)
> > > >
> > > > trace_tulip_receive(buf, size);
> > > >
> > > > - if (size < 14 || size > 2048 || s->rx_frame_len ||
> > > > tulip_rx_stopped(s)) {
> > > > + if (size < 14 || size > sizeof(s->rx_frame) - 4
> > > > + || s->rx_frame_len || tulip_rx_stopped(s)) {
> > > > return 0;
> > > > }
> > > >
> > > > @@ -275,7 +284,6 @@ static ssize_t
> > > tulip_receive_nc(NetClientState
> > > > *nc,
> > > > return tulip_receive(qemu_get_nic_opaque(nc),
> > buf, size);
> > > > }
> > > >
> > > > -
> > > > static NetClientInfo net_tulip_info = {
> > > > .type = NET_CLIENT_DRIVER_NIC,
> > > > .size = sizeof(NICState),
> > > > @@ -558,7 +566,7 @@ static void tulip_tx(TULIPState
> > *s, struct
> > > > tulip_descriptor *desc)
> > > > if ((s->csr[6] >> CSR6_OM_SHIFT) &
> > CSR6_OM_MASK) {
> > > > /* Internal or external Loopback */
> > > > tulip_receive(s, s->tx_frame,
> > s->tx_frame_len);
> > > > - } else {
> > > > + } else if (s->tx_frame_len <=
> > sizeof(s->tx_frame)) {
> > > > qemu_send_packet(qemu_get_queue(s->nic),
> > > > s->tx_frame, s->tx_frame_len);
> > > > }
> > > > @@ -570,23 +578,31 @@ static void tulip_tx(TULIPState
> > *s, struct
> > > > tulip_descriptor *desc)
> > > > }
> > > > }
> > > >
> > > > -static void tulip_copy_tx_buffers(TULIPState *s, struct
> > > > tulip_descriptor *desc)
> > > > +static int tulip_copy_tx_buffers(TULIPState *s, struct
> > > > tulip_descriptor *desc)
> > > > {
> > > > int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT)
> &
> > > > TDES1_BUF1_SIZE_MASK;
> > > > int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT)
> &
> > > > TDES1_BUF2_SIZE_MASK;
> > > >
> > > > + if (s->tx_frame_len + len1 >= sizeof(s->tx_frame)) {
> > > > + return -1;
> > > > + }
> > > > if (len1) {
> > > > pci_dma_read(&s->dev, desc->buf_addr1,
> > > > s->tx_frame + s->tx_frame_len, len1);
> > > > s->tx_frame_len += len1;
> > > > }
> > > >
> > > > + if (s->tx_frame_len + len2 >= sizeof(s->tx_frame)) {
> > > > + return -1;
> > > > + }
> > > > if (len2) {
> > > > pci_dma_read(&s->dev, desc->buf_addr2,
> > > > s->tx_frame + s->tx_frame_len, len2);
> > > > s->tx_frame_len += len2;
> > > > }
> > > > desc->status = (len1 + len2) ? 0 : 0x7fffffff;
> > > > +
> > > > + return 0;
> > > > }
> > > >
> > > > static void tulip_setup_filter_addr(TULIPState *s,
> > uint8_t
> > > *buf,
> > > > int n)
> > > > @@ -651,13 +667,15 @@ static uint32_t
> > tulip_ts(TULIPState *s)
> > > >
> > > > static void tulip_xmit_list_update(TULIPState *s)
> > > > {
> > > > +#define TULIP_DESC_MAX 128
> > > > + uint8_t i = 0;
> > > > struct tulip_descriptor desc;
> > > >
> > > > if (tulip_ts(s) != CSR5_TS_SUSPENDED) {
> > > > return;
> > > > }
> > > >
> > > > - for (;;) {
> > > > + for (i = 0; i < TULIP_DESC_MAX; i++) {
> > > > tulip_desc_read(s, s->current_tx_desc, &desc);
> > > > tulip_dump_tx_descriptor(s, &desc);
> > > >
> > > > @@ -675,10 +693,10 @@ static void
> > > > tulip_xmit_list_update(TULIPState *s)
> > > > s->tx_frame_len = 0;
> > > > }
> > > >
> > > > - tulip_copy_tx_buffers(s, &desc);
> > > > -
> > > > - if (desc.control & TDES1_LS) {
> > > > - tulip_tx(s, &desc);
> > > > + if (!tulip_copy_tx_buffers(s, &desc)) {
> > > > + if (desc.control & TDES1_LS) {
> > > > + tulip_tx(s, &desc);
> > > > + }
> > > > }
> > > > }
> > > > tulip_desc_write(s, s->current_tx_desc, &desc);
> > > > --
> > > > 2.25.1
> > > >
> > > >
> > >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 30237 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-03-27 3:44 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-23 12:20 [PATCH v6 0/2] net: tulip: add checks to avoid OOB access P J P
2020-03-23 12:20 ` [PATCH v6 1/2] net: tulip: check frame size and r/w data length P J P
2020-03-24 1:29 ` Li Qiang
2020-03-24 5:45 ` Jason Wang
2020-03-24 13:19 ` P J P
2020-03-24 14:54 ` Li Qiang
2020-03-27 2:09 ` Jason Wang
2020-03-27 2:35 ` Li Qiang
2020-03-27 2:53 ` Jason Wang
2020-03-27 3:43 ` Li Qiang
2020-03-26 4:34 ` P J P
2020-03-23 12:21 ` [PATCH v6 2/2] net: tulip: add .can_receive routine P J P
2020-03-24 2:04 ` Li Qiang
2020-03-24 5:44 ` Jason Wang
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.