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
>
>