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 <[email protected]> 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/[email protected]?part=3
