Re: [PATCH v2 6/8] iommu/arm-smmu-v3: Support IOMMU_GET_HW_INFO via struct arm_smmu_hw_info

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Sep 02, 2024 at 10:11:16AM +0000, Mostafa Saleh wrote:

> > What is the harm? Does exposing IDR data to userspace in any way
> > compromise the security or integrity of the system?
> > 
> > I think no - how could it?
> 
> I don’t see a clear harm or exploit with exposing IDRs, but IMHO we
> should deal with userspace with the least privilege principle and
> only expose what user space cares about (with sanitised IDRs or
> through another mechanism)

If the information is harmless then why hide it? We expose all kinds
of stuff to userspace, like most of the PCI config space for
instance. I think we need a reason. 

Any sanitization in the kernel will complicate everything because we
will get it wrong.

Let's not make things complicated without reasons. Intel and AMD are
exposing their IDR equivalents in this manner as well.

> For example, KVM doesn’t allow reading reading the CPU system
> registers to know if SVE(or other features) is supported but hides
> that by a CAP in KVM_CHECK_EXTENSION

Do you know why?

> > As the comments says, the VMM should not just blindly forward this to
> > a guest!
> 
> I don't think the kernel should trust userspace.

There is no trust. If the VMM blindly forwards the IDRS then the VMM
will find its VM's have issues. It is a functional bug, just as if the
VMM puts random garbage in its vIDRS.

The onl purpose of this interface is to provide information about the
physical hardware to the VMM.

> > The VMM needs to make its own IDR to reflect its own vSMMU
> > capabilities. It can refer to the kernel IDR if it needs to.
> > 
> > So, if the kernel is going to limit it, what criteria would you
> > propose the kernel use?
> 
> I agree that the VMM would create a virtual IDR for guest, but that
> doesn't have to be directly based on the physical one (same as CPU).

No one said it should be. In fact the comment explicitly says not to
do that.

The VMM is expected to read out of the physical IDR any information
that effects data structures that are under direct guest control.

For instance anything that effects the CD on downwards. So page sizes,
IAS limits, etc etc etc. Anything that effects assigned invalidation
queues. Anything that impacts errata the VM needs to be aware of.

If you sanitize it then you will hide information that someone will
need at some point, then we have go an unsanitize it, then add feature
flags.. It is a pain.

Jason