| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250108091126.5a84266e@collabora.com>
Date: Wed, 8 Jan 2025 09:11:26 +0100
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Florent Tomasin <florent.tomasin@....com>
Cc: Steven Price <steven.price@....com>, Liviu Dudau <liviu.dudau@....com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
<mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
<airlied@...il.com>, Simona Vetter <simona@...ll.ch>, <nd@....com>,
<dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/panthor: Fix invalid handling of AS_LOCKADDR
On Tue, 7 Jan 2025 17:27:31 +0000
Florent Tomasin <florent.tomasin@....com> wrote:
> Arm Mali GPUs require AS_LOCKADDR region to be 32KB
> aligned, and does not support a size greater than
> the one specified by the HW property:
> `GPU_MMU_FEATURES_VA_BITS()`.
>
> NOTES:
> - The size limitation is implementation defined.
> - Invalid alignment or size can result in an HW
> undefined behaviour.
>
> This patch modifies `lock_region()` to retrieve
> the maximum region size based on the HW property:
> `mmu_features`, and returns an error code if the
> requested size is not compliant with the HW
> limitation.
>
> In addition, the function will guaranty the region
> is always 32KB aligned.
>
> Signed-off-by: Florent Tomasin <florent.tomasin@....com>
> ---
> drivers/gpu/drm/panthor/panthor_mmu.c | 37 ++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index c39e3eb1c15d..e834bc4d9a52 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -533,15 +533,20 @@ static int write_cmd(struct panthor_device *ptdev, u32 as_nr, u32 cmd)
> return status;
> }
>
> -static void lock_region(struct panthor_device *ptdev, u32 as_nr,
> - u64 region_start, u64 size)
> +static int lock_region(struct panthor_device *ptdev, u32 as_nr,
> + u64 region_start, u64 size)
> {
> + u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
> + u64 full_va_range = 1ull << va_bits;
Looks like we have a few places where we need the full_va_range, so I'd
be in favor of adding the following helper:
static u64 mmu_va_range(const struct panthor_device *ptdev)
{
u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
return 1ull << va_bits;
}
> u8 region_width;
> u64 region;
> u64 region_end = region_start + size;
>
> if (!size)
> - return;
> + return 0;
> +
> + if (drm_WARN_ON(&ptdev->base, region_end > full_va_range))
> + return -EFAULT;
How about we keep the function void and adjust the region_end to avoid
the undefined behavior?
>
> /*
> * The locked region is a naturally aligned power of 2 block encoded as
> @@ -552,7 +557,8 @@ static void lock_region(struct panthor_device *ptdev, u32 as_nr,
> * zeroed and ends with the bit (and subsequent bits) set to one.
> */
> region_width = max(fls64(region_start ^ (region_end - 1)),
> - const_ilog2(AS_LOCK_REGION_MIN_SIZE)) - 1;
> + const_ilog2(AS_LOCK_REGION_MIN_SIZE));
> +
I agree that the name doesn't really reflect its content, and doing the
minus(1) mod when preparing the region value is clearer, but it doesn't
seem like a bug to me, and is unrelated to the other changes in this
patch (actually, it's not even mentioned in the commit message). For
all these reasons, I'd put it in a separate patch.
>
> /*
> * Mask off the low bits of region_start (which would be ignored by
> @@ -560,21 +566,25 @@ static void lock_region(struct panthor_device *ptdev, u32 as_nr,
> */
> region_start &= GENMASK_ULL(63, region_width);
>
> - region = region_width | region_start;
> + region = (region_width - 1) | region_start;
>
> /* Lock the region that needs to be updated */
> gpu_write(ptdev, AS_LOCKADDR_LO(as_nr), lower_32_bits(region));
> gpu_write(ptdev, AS_LOCKADDR_HI(as_nr), upper_32_bits(region));
> write_cmd(ptdev, as_nr, AS_COMMAND_LOCK);
> +
> + return 0;
> }
>
> static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
> u64 iova, u64 size, u32 op)
> {
> + int ret = 0;
> +
> lockdep_assert_held(&ptdev->mmu->as.slots_lock);
>
> if (as_nr < 0)
> - return 0;
> + return ret;
>
> /*
> * If the AS number is greater than zero, then we can be sure
> @@ -583,7 +593,10 @@ static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
> */
>
> if (op != AS_COMMAND_UNLOCK)
> - lock_region(ptdev, as_nr, iova, size);
> + ret = lock_region(ptdev, as_nr, iova, size);
> +
> + if (ret)
> + return ret;
>
> /* Run the MMU operation */
> write_cmd(ptdev, as_nr, op);
> @@ -608,9 +621,12 @@ static int mmu_hw_do_operation(struct panthor_vm *vm,
> static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
> u64 transtab, u64 transcfg, u64 memattr)
> {
> + u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
> + u64 full_va_range = 1ull << va_bits;
> int ret;
>
> - ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
> + ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0,
> + full_va_range, AS_COMMAND_FLUSH_MEM);
> if (ret)
> return ret;
>
> @@ -628,9 +644,12 @@ static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
>
> static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
> {
> + u32 va_bits = GPU_MMU_FEATURES_VA_BITS(ptdev->gpu_info.mmu_features);
> + u64 full_va_range = 1ull << va_bits;
> int ret;
>
> - ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
> + ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0,
> + full_va_range, AS_COMMAND_FLUSH_MEM);
> if (ret)
> return ret;
>
Powered by blists - more mailing lists