From: Peter Maydell <email@example.com>
To: "Cédric Le Goater" <firstname.lastname@example.org>
Cc: Andrew Jeffery <email@example.com>,
Mauro Matteo Cascella <firstname.lastname@example.org>,
QEMU Developers <email@example.com>,
ziming zhang <firstname.lastname@example.org>
Subject: Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()
Date: Mon, 13 Jul 2020 17:15:36 +0100 [thread overview]
Message-ID: <CAFEAcA9=_RC0w-EgjdPw=UWXZ-ufHjkeDWTMj_jXfSQL8G9GHA@mail.gmail.com> (raw)
On Mon, 13 Jul 2020 at 15:19, Cédric Le Goater <email@example.com> 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 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
* zero-byte tx descriptors might just be marked "done" and
skipped over since they have no actual data
* 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)
* 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. 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'.
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).
(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.
next prev parent reply other threads:[~2020-07-13 16:16 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-10 8:54 [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx() Mauro Matteo Cascella
2020-07-10 11:33 ` Peter Maydell
2020-07-10 13:20 ` Mauro Matteo Cascella
2020-07-13 14:19 ` Cédric Le Goater
2020-07-13 16:15 ` Peter Maydell [this message]
2020-07-29 15:15 ` Cédric Le Goater
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.