[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()
From: |
Cédric Le Goater |
Subject: |
Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx() |
Date: |
Wed, 29 Jul 2020 17:15:09 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 |
Sorry for the late answer.
On 7/13/20 6:15 PM, Peter Maydell wrote:
> On Mon, 13 Jul 2020 at 15:19, Cédric Le Goater <clg@kaod.org> wrote:
>> On 7/10/20 1:33 PM, Peter Maydell wrote:
>>> Andrew, Cedric: do you have the datasheet for this device? Do you
>>> know if we should also be flagging the error back to the
>>> guest somehow?
>>
>> zero is the only invalid size of a transmit buffer and the specs does
>> not have any special information on which bit to raise in that case.
>
> I found a datasheet which might or might not be the equivalent
> bit of hardware -- does your datasheet have a note on the
> TXBUF_SIZE field of a tx descriptor that says "When the size is 0,
> the descriptor would be discarded" ? (Though I found another
> random doc that just says it's illegal...)
I only have :
TXBUF SIZE: Transmit buffer size in byte
The transmit buffer size can not be zero.
>> I think FTGMAC100_INT_NO_NPTXBUF (transmit buffer unavailable) is our
>> best option and we should add an extra 'len == 0' test in front of
>> the dma_memory_read() call to raise it. A zero length is not considered
>> bogus by dma_memory_read() it seems.
>
> My best guess at "what the hardware does" here would be:
> * TXBUF_SIZE in a tx descriptor can be anything: the h/w
> would happily allow you to assemble a tx packet with a
> whole series of 1-byte sized buffers, each with its own
> tx descriptor
yes.
> * zero-byte tx descriptors might just be marked "done" and
> skipped over since they have no actual data
yes.
> * any checking on max/min lengths would be done
> only on the accumulated total-packet length (we do this
> this way already for the frame-too-big check)
yes.
> * I suspect "transmit buffer unavailable" means "the ethernet
> controller needs more data but the next tx descriptor
> is still marked as owned by the guest" -- this is certainly
> what we currently do with it, and that doesn't seem like
> the best thing to signal for the "tx packet too small"
> case. It's possible that the hardware simply sends out a
> runt packet of some form if the software tells it to do
> that. My vote would be for handling it with XPKT_LOST,
> the same way we do for over-large frames.
XPKT_LOST means that packets are lost due to late/excessive
collision. I have used this status for large frames because
not other bits made sense.
> This probably
> is not what the hardware does but at least it's a
> coherent thing that the guest might be expecting to have
> happen for a tx attempt and it matches the fact that we
> really are not going to put it on the 'wire'.
I agree.
>
> Side note: I suspect that any failures from
> dma_memory_read() and dma_memory_write() should be
> reported as AHB_ERR (currently we have a mix of
> ignoring them or using NO_NPTXBUF).
AHB_ERR is not used in the Aspeed implementation. I checked with
them. But I think it makes sense for other implementations, so we
can add this status for Linux.
NO_NPTXBUF means a lack a transmit descriptors.
XPKT_LOST is our best choice then.
Thanks,
C.
> (It would in theory be possible to test some of these edge
> cases on real hardware, but that kind of bare-metal test
> case is usually a pain to put together and way overkill
> for this situation, so I don't think we should bother.)
>
>> Is address zero considered bogus ?
>> If not, we need to check that also.
>
> Writes to address 0 are fine, it is not a special physical address.
>
> thanks
> -- PMM
>