Re: [PATCH v7 3/7] drm/tyr: add Memory Management Unit (MMU) support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Early returns in the MMU eviction path on hardware timeouts skip hardware disablement, leading to a use-after-free of the page tables.
- [High] The MAIR attribute parsing logic incorrectly classifies valid Write-Back memory as device memory and Non-cacheable memory as Write-Back due to flawed bitwise checks.
- [High] Bit-shift panic and integer underflow when `as_start_update` is called with an empty or wrapping region.
- [High] Hardware AS slot assignment fails when the AS_PRESENT mask has holes because it maps a contiguous logical index directly to a physical slot.
- [High] VM updates crossing a 4GB boundary fail because the calculated lock region size exceeds the 5-bit LOCKADDR.size field.
- [High] `AddressSpaceManager::as_flush` does not wait for the hardware `FlushPt` command to complete.
- [High] `AddressSpaceManager::as_start_update` does not wait for the hardware `Lock` command to complete.
- [Medium] Creating a VM with unsupported `va_bits` causes a panic due to arithmetic underflow.
--

--- Patch [3]: [PATCH v7 3/7] drm/tyr: add Memory Management Unit (MMU) support ---
commit 3d38e41a867c897b123982cd2a02b0eb472ce1c9
Author: Boris Brezillon <boris.brezillon@collabora.com>

drm/tyr: add Memory Management Unit (MMU) support

Add Memory Management Unit (MMU) support in Tyr. The MMU module wraps a
SlotManager instance to allocate MMU address-space slots for use by
virtual memory (VM) address spaces. The MMU's SlotManager uses an
AddressSpaceManager to handle the hardware-specific callbacks. For
example, the AddressSpaceManager activates and evicts VMs from slots by
writing commands to the MMU registers.

Add an implementation block for the MMU's MEMATTR register to provide
a method for translating the Memory Attribute Indirection Register (MAIR)
format from the pagetable configuration to a format understood by the MMU.

Create an mmu instance during probe, it will be used by subsequent patches
in this series.

Wrap the iomem stored in TyrDrmRegistrationData in an Arc. The iomem
is stored in the mmu through its AddressSpaceManager. In anticipation
of the iomem also being stored in the firmware object, set up shared
ownership of the iomem now.

Update Kconfig to add the new MMU and IOMMU dependencies required
by this MMU module.

> diff --git a/drivers/gpu/drm/tyr/mmu/address_space.rs b/drivers/gpu/drm/tyr/mmu/address_space.rs
> index 0000000000000..20dca96f4a1ee 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/mmu/address_space.rs

[ ... ]

> +impl<'bound> VmAsData<'bound> {

[ ... ]

> +    fn as_config(&self) -> Result<AddressSpaceConfig> {
> +        let pt = &self.page_table;

[ ... ]

> +        let ina_bits_field_value = 55 - self.va_bits;

[Severity: Medium]
Can this arithmetic operation in VmAsData::as_config() underflow and cause a
kernel panic if a VM is created with va_bits greater than 55?

[ ... ]

> +impl<'bound> AddressSpaceManager<'bound> {

[ ... ]

> +    fn validate_as_slot(&self, as_nr: usize) -> Result {
> +        if as_nr >= MAX_AS {
> +            pr_err!("AS slot {} out of valid range (max {})\n", as_nr, MAX_AS);
> +            return Err(EINVAL);
> +        }
> +
> +        if (self.as_present & (1 << as_nr)) == 0 {

[Severity: High]
Does this logic in AddressSpaceManager::validate_as_slot() fail if the
AS_PRESENT mask contains holes?

During probe, Mmu::new() sets up the generic SlotManager with the total
count of present bits:

Mmu::new()
    let slot_count = present.count_ones().try_into()?;
    let as_manager = AddressSpaceManager::new(iomem.into(), present)?;
    ...
    SlotManager::new(as_manager, slot_count)?

This assigns contiguous logical indices (e.g. 0, 1, 2) to slots. If the mask is
sparse (like 0b1011), checking the physical bit directly with the logical
index might falsely fail validation when as_nr maps to a missing bit.

[ ... ]

> +    fn as_disable(&mut self, as_nr: usize) -> Result {
> +        self.validate_as_slot(as_nr)?;
> +
> +        // Flush AS before disabling
> +        self.as_send_cmd_and_wait(as_nr, MmuCommand::FlushMem)?;

[Severity: Critical]
If as_send_cmd_and_wait() times out and returns an error, does the ? operator
cause AddressSpaceManager::as_disable() and the evict() path to abort early?

If eviction aborts without zeroing the TRANSTAB registers, the hardware slot
remains active and pointing to page tables that the OS is about to free, which
could lead to a use-after-free of those page tables by the hardware.

[ ... ]

> +    fn as_start_update(&mut self, as_nr: usize, region: &Range<u64>) -> Result {
> +        self.validate_as_slot(as_nr)?;

[ ... ]

> +        let addr_xor = region.start ^ (region.end - 1);

[Severity: High]
Can this cause an arithmetic underflow and out-of-bounds bit shift panic if
AddressSpaceManager::as_start_update() is called with a region where
region.start and region.end are both 0?

If region.end is 0, region.end - 1 wraps to u64::MAX. addr_xor becomes
very large, leading to region_size_log2 becoming 64.

Later in the function:
    let lockaddr_base = region.start & !((1u64 << lock_region_log2) - 1);

Shifting 1u64 by 64 will deterministically panic in Rust.

> +        let region_size_log2 = 64 - addr_xor.leading_zeros() as u8;
> +
> +        let lock_region_log2 = core::cmp::max(region_size_log2, lock_region_min_size_log2);

[ ... ]

> +        let lockaddr_size = lock_region_log2 - 1;

[ ... ]

> +        let lockaddr_val = LOCKADDR::zeroed()
> +            .try_with_size(lockaddr_size)?

[Severity: High]
Are valid VM updates that cross a 4GB boundary rejected here?

If an update crosses a 4GB boundary (e.g. from 0xFFFFF000 to 0x100001000),
the XOR of the start and end addresses sets bit 32. This makes
region_size_log2 equal to 33, and lockaddr_size is calculated as 32.

Since the hardware field is 5 bits (max 31), try_with_size() will return
an error instead of proceeding.

[ ... ]

> +        io.write(
> +            LOCKADDR_HI::try_at(as_nr).ok_or(EINVAL)?,
> +            LOCKADDR_HI::from_raw((lockaddr_val >> 32) as u32),
> +        );
> +
> +        self.as_send_cmd(as_nr, MmuCommand::Lock)
> +    }

[Severity: High]
Does this code need to wait for the Lock command to complete?

Since as_send_cmd() returns immediately, the CPU might begin modifying the
page tables before the hardware has actually locked the TLB, breaking atomicity.

[ ... ]

> +    fn as_flush(&mut self, as_nr: usize) -> Result {
> +        self.validate_as_slot(as_nr)?;
> +        self.as_send_cmd(as_nr, MmuCommand::FlushPt)
> +    }

[Severity: High]
Does this code need to wait for the FlushPt command to complete?

If as_flush() returns before the hardware completes the flush, the CPU could
proceed to unmap or free pages while the GPU is still processing them, creating
a TLB use-after-free scenario.

> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index 831357a8ef87d..eae9213fb1c2b 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs

[ ... ]

> +        impl MEMATTR {

[ ... ]

> +            fn is_device_memory(mair_attr: u8) -> bool {
> +                // In AArch64 MAIR, device memory has bits [1:0] of outer nibble = 0.
> +                let outer = mair_attr >> 4;
> +                (outer & 0x3) == 0
> +            }

[Severity: High]
Can MEMATTR::is_device_memory() incorrectly classify valid memory attributes
as device memory?

If a buffer is mapped using a MAIR attribute where the outer cache policy is
Write-Back but No-Allocate (e.g. 0xCC), outer & 0x3 would evaluate to
12 & 3 == 0. This incorrectly categorizes the memory as device memory.

[ ... ]

> +            fn is_writeback_cacheable(mair_attr: u8) -> bool {
> +                let outer = mair_attr >> 4;
> +                let inner = mair_attr & Self::ARM_MAIR_INNER_MASK;
> +
> +                (outer & Self::ARM_MAIR_WRITE_BACK) != 0 && (inner & Self::ARM_MAIR_WRITE_BACK) != 0
> +            }

[Severity: High]
Does MEMATTR::is_writeback_cacheable() mistakenly identify memory as Write-Back
when the inner cache policy is Non-Cacheable?

For an attribute like 0x74 (Outer Write-Back, Inner Non-Cacheable), outer & 4
and inner & 4 are both non-zero, violating the requested cache policy by
mapping Inner Non-Cacheable memory as fully Write-Back.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-fw-boot-b4-v7-0-afd8dc1494dd@collabora.com?part=3