Re: [PATCH RFC 2/2] audit accesses to unassigned PCI config regions

[Chathura Rajapaksha <chathura.abeyrathne.lk@gmail.com>]

Hi Bjorn and Paul,

Thank you for your comments, and sorry for the late reply.

On Mon, Apr 28, 2025 at 11:05 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> Add blank line between paragraphs.

> Use imperative mood ("Introduce" instead of "This patch introduces
> ..." and "Add ..." instead of "A new type has been introduced").

> Simplify this patch by adding "blocked" in the first patch.  Then you
> won't have to touch the permission checking that is unrelated to the
> audit logging.  Consider adding a helper to do the checking and return
> "blocked" so it doesn't clutter vfio_config_do_rw().

I will address the above comments in the next revision.

On Fri, May 16, 2025 at 4:41 PM Paul Moore <paul@paul-moore.com> wrote:
> I try to encourage people to put a sample audit record in the commit
> description as it helps others, even those not overly familiar with the
> Linux kernel, know what to expect in the audit log and provide feedback.

> > +static const char * const vfio_audit_str[VFIO_AUDIT_MAX] = {
> > +     [VFIO_AUDIT_READ]  = "READ",
> > +     [VFIO_AUDIT_WRITE] = "WRITE",
> > +};
>
> We generally don't capitalize things like this in audit, "read" and
> "write" would be preferred.

I will address the above comments in the next revision.
The following is the expected audit message when a write is performed
to an unassigned PCI config region:

  device=0000:01:00.1 access=WRITE offset=0x298 size=1 blocked=0

> In the commit description you talk about a general PCIe device issue
> in the first paragraph before going into the specifics of the VFIO
> driver.  That's all well and good, but it makes me wonder if this
> audit code above is better done as a generic PCI function that other
> PCI drivers could use if they had similar concerns?  Please correct
> me if I'm wrong, but other than symbol naming I don't see anyting
> above which is specific to VFIO.  Thoughts?

While the issue is independent of VFIO, the security and availability
concerns arise when guests are able to write to unassigned PCI config
regions on devices passed through using VFIO. That's why we thought it
would be better to audit these accesses in the VFIO driver. Given this
context, do you think it would be more appropriate to audit these
accesses through a generic PCI function instead?

> Beyond that, I might also change the "access=" field to "op=" as we
> already use the "op=" field name for similar things in audit, it would
> be good to leverage that familiarity here.  Similarly using "res=",
> specifically "res=0" for failure/blocked or "res=1" allowed, would
> better fit with audit conventions.

Thanks for the suggestions, I will address these in the next revision.

Regards,
Chathura