Avoid unaligned fetch in ladr_match()

[PATCH] Avoid unaligned fetch in ladr_match()

Posted by Nick Briggs 9 months, 4 weeks ago

There is no guarantee that the PCNetState is allocated such that
csr[8] is allocated on an 8-byte boundary.  Since not all hosts are
capable of unaligned fetches the 16-bit elements need to be fetched
individually to avoid a potential fault.  Closes issue #2143

Signed-off-by: Nick Briggs <nicholas.h.briggs@gmail.com>
---
 hw/net/pcnet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 494eab8479..ad675ab29d 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -632,7 +632,7 @@ static inline int ladr_match(PCNetState *s, const uint8_t *buf, int size)
 {
     struct qemu_ether_header *hdr = (void *)buf;
     if ((*(hdr->ether_dhost)&0x01) &&
-        ((uint64_t *)&s->csr[8])[0] != 0LL) {
+        (s->csr[8] | s->csr[9] | s->csr[10] | s->csr[11]) != 0) {
         uint8_t ladr[8] = {
             s->csr[8] & 0xff, s->csr[8] >> 8,
             s->csr[9] & 0xff, s->csr[9] >> 8,
-- 
2.31.1

Re: [PATCH] Avoid unaligned fetch in ladr_match()

Posted by Peter Maydell 9 months, 4 weeks ago

On Thu, 1 Feb 2024 at 18:11, Nick Briggs <nicholas.h.briggs@gmail.com> wrote:
>
> There is no guarantee that the PCNetState is allocated such that
> csr[8] is allocated on an 8-byte boundary.  Since not all hosts are
> capable of unaligned fetches the 16-bit elements need to be fetched
> individually to avoid a potential fault.  Closes issue #2143


The way to mark that a commit fixes an issue is to have:

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2143

just above the Signed-off-by: line.

> Signed-off-by: Nick Briggs <nicholas.h.briggs@gmail.com>
> ---
>  hw/net/pcnet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
> index 494eab8479..ad675ab29d 100644
> --- a/hw/net/pcnet.c
> +++ b/hw/net/pcnet.c
> @@ -632,7 +632,7 @@ static inline int ladr_match(PCNetState *s, const uint8_t *buf, int size)
>  {
>      struct qemu_ether_header *hdr = (void *)buf;
>      if ((*(hdr->ether_dhost)&0x01) &&
> -        ((uint64_t *)&s->csr[8])[0] != 0LL) {
> +        (s->csr[8] | s->csr[9] | s->csr[10] | s->csr[11]) != 0) {
>          uint8_t ladr[8] = {
>              s->csr[8] & 0xff, s->csr[8] >> 8,
>              s->csr[9] & 0xff, s->csr[9] >> 8,

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(The whole function feels kind of clunky, eg I have
a feeling you could index directly into the csr[]
arrays rather than constructing the ladr[] byte array,
but this is the simple fix for the problem at hand.)

thanks
-- PMM