who's maintaining amd_iommu.c these days?

who's maintaining amd_iommu.c these days?

[Peter Maydell]

Coverity points out some problems with hw/i386/amd_iommu.c's event
logging code -- specifically, CID 1487115 1487116 1487190 1487200
1487232 1487258 are all the same basic problem, which is that various
functions declare a local "uint64_t evt[4]", populate only some
bits of it and then write it to guest memory, so we end up using
uninitialized host data and leaking it to the guest. I was going to
write a fix for this, but in looking at the code I noticed that
it has more extensive problems:

(1) these functions allocate an array of 4 64-bit values,
but we only copy 2 to the guest, because AMDVI_EVENT_LEN is 16.
Looking at the spec, I think that the length is right and it's
really 4 32-bit values (or 2 64-bit values, if you like).

(2) There are host-endianness bugs, because we assemble the
event as a set of host-endianness values but then write them
to guest memory as a bag-of-bytes with dma_memory_write()

(3) amdvi_encode_event() is throwing away most of its
"addr" argument, because it calls
  amdvi_setevent_bits(evt, addr, 63, 64) apparently intending
that to write 64 bits starting at 63 bits into the packet, but
the amdvi_setevent_bits() function only ever updates one
uint64_t in the array, so it will in fact write bit 63 and
nothing else.

(4) The claimed bit layout of the event structure doesn't
match up with the one in the spec document I found. This
could be because I found a document for some other bit
of hardware, of course.

Anyway, adding all these up, the event logging probably
needs a bit of a restructuring, and that should ideally be
done by somebody who (a) knows the hardware we're emulating
here and (b) is in a position to test things. Any volunteers?

thanks
-- PMM

Re: who's maintaining amd_iommu.c these days?

[Peter Xu]

On Thu, Mar 31, 2022 at 05:01:52PM +0100, Peter Maydell wrote:
> Coverity points out some problems with hw/i386/amd_iommu.c's event
> logging code -- specifically, CID 1487115 1487116 1487190 1487200
> 1487232 1487258 are all the same basic problem, which is that various
> functions declare a local "uint64_t evt[4]", populate only some
> bits of it and then write it to guest memory, so we end up using
> uninitialized host data and leaking it to the guest. I was going to
> write a fix for this, but in looking at the code I noticed that
> it has more extensive problems:
> 
> (1) these functions allocate an array of 4 64-bit values,
> but we only copy 2 to the guest, because AMDVI_EVENT_LEN is 16.
> Looking at the spec, I think that the length is right and it's
> really 4 32-bit values (or 2 64-bit values, if you like).
> 
> (2) There are host-endianness bugs, because we assemble the
> event as a set of host-endianness values but then write them
> to guest memory as a bag-of-bytes with dma_memory_write()
> 
> (3) amdvi_encode_event() is throwing away most of its
> "addr" argument, because it calls
>   amdvi_setevent_bits(evt, addr, 63, 64) apparently intending
> that to write 64 bits starting at 63 bits into the packet, but
> the amdvi_setevent_bits() function only ever updates one
> uint64_t in the array, so it will in fact write bit 63 and
> nothing else.
> 
> (4) The claimed bit layout of the event structure doesn't
> match up with the one in the spec document I found. This
> could be because I found a document for some other bit
> of hardware, of course.
> 
> Anyway, adding all these up, the event logging probably
> needs a bit of a restructuring, and that should ideally be
> done by somebody who (a) knows the hardware we're emulating
> here and (b) is in a position to test things. Any volunteers?

Copying some AMD developers (from where I saw the last patches from)...

-- 
Peter Xu

Re: who's maintaining amd_iommu.c these days?

[Jason Wang]

On Fri, Apr 1, 2022 at 2:30 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Mar 31, 2022 at 05:01:52PM +0100, Peter Maydell wrote:
> > Coverity points out some problems with hw/i386/amd_iommu.c's event
> > logging code -- specifically, CID 1487115 1487116 1487190 1487200
> > 1487232 1487258 are all the same basic problem, which is that various
> > functions declare a local "uint64_t evt[4]", populate only some
> > bits of it and then write it to guest memory, so we end up using
> > uninitialized host data and leaking it to the guest. I was going to
> > write a fix for this, but in looking at the code I noticed that
> > it has more extensive problems:
> >
> > (1) these functions allocate an array of 4 64-bit values,
> > but we only copy 2 to the guest, because AMDVI_EVENT_LEN is 16.
> > Looking at the spec, I think that the length is right and it's
> > really 4 32-bit values (or 2 64-bit values, if you like).
> >
> > (2) There are host-endianness bugs, because we assemble the
> > event as a set of host-endianness values but then write them
> > to guest memory as a bag-of-bytes with dma_memory_write()
> >
> > (3) amdvi_encode_event() is throwing away most of its
> > "addr" argument, because it calls
> >   amdvi_setevent_bits(evt, addr, 63, 64) apparently intending
> > that to write 64 bits starting at 63 bits into the packet, but
> > the amdvi_setevent_bits() function only ever updates one
> > uint64_t in the array, so it will in fact write bit 63 and
> > nothing else.
> >
> > (4) The claimed bit layout of the event structure doesn't
> > match up with the one in the spec document I found. This
> > could be because I found a document for some other bit
> > of hardware, of course.
> >
> > Anyway, adding all these up, the event logging probably
> > needs a bit of a restructuring, and that should ideally be
> > done by somebody who (a) knows the hardware we're emulating
> > here and (b) is in a position to test things. Any volunteers?
>
> Copying some AMD developers (from where I saw the last patches from)...

Btw, the AMD IOMMU seems not to work for a while (just boot it with
virtio-blk and it still doesn't work).

Thanks

>
> --
> Peter Xu
>

Re: who's maintaining amd_iommu.c these days?

[Wei Huang]

On 3/31/22 21:09, Jason Wang wrote:
> On Fri, Apr 1, 2022 at 2:30 AM Peter Xu <peterx@redhat.com> wrote:
>>
>> On Thu, Mar 31, 2022 at 05:01:52PM +0100, Peter Maydell wrote:
>>> Coverity points out some problems with hw/i386/amd_iommu.c's event
>>> logging code -- specifically, CID 1487115 1487116 1487190 1487200
>>> 1487232 1487258 are all the same basic problem, which is that various
>>> functions declare a local "uint64_t evt[4]", populate only some
>>> bits of it and then write it to guest memory, so we end up using
>>> uninitialized host data and leaking it to the guest. I was going to
>>> write a fix for this, but in looking at the code I noticed that
>>> it has more extensive problems:
>>>
>>> (1) these functions allocate an array of 4 64-bit values,
>>> but we only copy 2 to the guest, because AMDVI_EVENT_LEN is 16.
>>> Looking at the spec, I think that the length is right and it's
>>> really 4 32-bit values (or 2 64-bit values, if you like).

Yes, amd event log entry size is 16 bytes. This should be easy to fixed.

>>>
>>> (2) There are host-endianness bugs, because we assemble the
>>> event as a set of host-endianness values but then write them
>>> to guest memory as a bag-of-bytes with dma_memory_write()

I doubt people will enable AMD IOMMUs in QEMU on non-x86 host. 
Nevertheless this can be problematic and should be addressed.

>>>
>>> (3) amdvi_encode_event() is throwing away most of its
>>> "addr" argument, because it calls
>>>    amdvi_setevent_bits(evt, addr, 63, 64) apparently intending
>>> that to write 64 bits starting at 63 bits into the packet, but
>>> the amdvi_setevent_bits() function only ever updates one
>>> uint64_t in the array, so it will in fact write bit 63 and
>>> nothing else.

Agreed

>>>
>>> (4) The claimed bit layout of the event structure doesn't
>>> match up with the one in the spec document I found. This
>>> could be because I found a document for some other bit
>>> of hardware, of course.
Could you elaborate? Are you referring to amdvi_setevent_bits(evt, info, 
55, 8)?

>>>
>>> Anyway, adding all these up, the event logging probably
>>> needs a bit of a restructuring, and that should ideally be
>>> done by somebody who (a) knows the hardware we're emulating
>>> here and (b) is in a position to test things. Any volunteers?
>>
>> Copying some AMD developers (from where I saw the last patches from)...
> 
> Btw, the AMD IOMMU seems not to work for a while (just boot it with
> virtio-blk and it still doesn't work).
> 

This can be related to address space notifier problem I encountered. We 
will take care of these issues mentioned in this email thread.

> Thanks
> 
>>
>> --
>> Peter Xu
>>
> 

Re: who's maintaining amd_iommu.c these days?

[Peter Maydell]

On Fri, 1 Apr 2022 at 17:11, Wei Huang <wei.huang2@amd.com> wrote:
>
>
>
> On 3/31/22 21:09, Jason Wang wrote:
> > On Fri, Apr 1, 2022 at 2:30 AM Peter Xu <peterx@redhat.com> wrote:
> >>
> >> On Thu, Mar 31, 2022 at 05:01:52PM +0100, Peter Maydell wrote:
> >>> (4) The claimed bit layout of the event structure doesn't
> >>> match up with the one in the spec document I found. This
> >>> could be because I found a document for some other bit
> >>> of hardware, of course.
> Could you elaborate? Are you referring to amdvi_setevent_bits(evt, info,
> 55, 8)?

https://www.amd.com/system/files/TechDocs/48882_IOMMU.pdf
was the spec I found through random googling, but the event
structure layouts in chapter 2.5 of that document aren't
at all like the one that amdvi_encode_event() is using.
Maybe that's the wrong spec, as I say.

thanks
-- PMM

Re: who's maintaining amd_iommu.c these days?

[Wei Huang]

On 4/1/22 11:14, Peter Maydell wrote:
> On Fri, 1 Apr 2022 at 17:11, Wei Huang <wei.huang2@amd.com> wrote:
>>
>>
>>
>> On 3/31/22 21:09, Jason Wang wrote:
>>> On Fri, Apr 1, 2022 at 2:30 AM Peter Xu <peterx@redhat.com> wrote:
>>>>
>>>> On Thu, Mar 31, 2022 at 05:01:52PM +0100, Peter Maydell wrote:
>>>>> (4) The claimed bit layout of the event structure doesn't
>>>>> match up with the one in the spec document I found. This
>>>>> could be because I found a document for some other bit
>>>>> of hardware, of course.
>> Could you elaborate? Are you referring to amdvi_setevent_bits(evt, info,
>> 55, 8)?
> 
> https://www.amd.com/system/files/TechDocs/48882_IOMMU.pdf
> was the spec I found through random googling, but the event
> structure layouts in chapter 2.5 of that document aren't
> at all like the one that amdvi_encode_event() is using.
> Maybe that's the wrong spec, as I say.

The spec is up-to-date. But it seems amdvi_setevent_bits(evt, info, 55, 
8) is problematic. Use amdvi_log_illegaldevtab_error() as an example:

     info |= AMDVI_EVENT_ILLEGAL_DEVTAB_ENTRY;
     amdvi_encode_event(evt, devid, addr, info);
     amdvi_log_event(s, evt);

info is encoded with AMDVI_EVENT_ILLEGAL_DEVTAB_ENTRY (1U << 12) and is 
used by amdvi_encode_event() via amdvi_setevent_bits(evt, info, 55, 8) 
into event log bits[63:55]. This should instead be written into 
bits[63:48] to match ILLEGAL_DEV_TABLE_ENTRY event format in Chapter 2.5.2.

> 
> thanks
> -- PMM