| 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: <92e5ebbd-9af1-c7e3-6d9e-7ea2958ad9c8@intel.com>
Date: Tue, 16 Aug 2022 18:42:12 +0900
From: Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>
To: Jani Nikula <jani.nikula@...el.com>,
<intel-gfx@...ts.freedesktop.org>
CC: <linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
<mchehab@...nel.org>, <chris@...is-wilson.co.uk>,
<matthew.auld@...el.com>, <thomas.hellstrom@...ux.intel.com>,
<nirmoy.das@...el.com>, <airlied@...ux.ie>, <daniel@...ll.ch>,
<andi.shyti@...ux.intel.com>, <andrzej.hajda@...el.com>
Subject: Re: [PATCH v6 5/8] drm/i915: Check for integer truncation on the
configuration of ttm place
On 8/15/22 5:03 PM, Jani Nikula wrote:
> On Sat, 13 Aug 2022, Gwan-gyeong Mun <gwan-gyeong.mun@...el.com> wrote:
>> There is an impedance mismatch between the first/last valid page
>> frame number of ttm place in unsigned and our memory/page accounting in
>> unsigned long.
>> As the object size is under the control of userspace, we have to be prudent
>> and catch the conversion errors.
>> To catch the implicit truncation as we switch from unsigned long to
>> unsigned, we use overflows_type check and report E2BIG or overflow_type
>> prior to the operation.
>>
>> v3: Not to change execution inside a macro. (Mauro)
>> Add safe_conversion_gem_bug_on() macro and remove temporal
>> SAFE_CONVERSION() macro.
>> v4: Fix unhandled GEM_BUG_ON() macro call from safe_conversion_gem_bug_on()
>> v6: Fix to follow general use case for GEM_BUG_ON(). (Jani)
>>
>> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>
>> Cc: Chris Wilson <chris@...is-wilson.co.uk>
>> Cc: Matthew Auld <matthew.auld@...el.com>
>> Cc: Thomas Hellström <thomas.hellstrom@...ux.intel.com>
>> Cc: Jani Nikula <jani.nikula@...el.com>
>> Reviewed-by: Nirmoy Das <nirmoy.das@...el.com> (v2)
>> Reviewed-by: Mauro Carvalho Chehab <mchehab@...nel.org> (v3)
>> Reported-by: kernel test robot <lkp@...el.com>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@...el.com> (v5)
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 6 +++---
>> drivers/gpu/drm/i915/intel_region_ttm.c | 22 +++++++++++++++++++---
>> 2 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 9f2be1892b6c..30f488712abe 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -140,14 +140,14 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr,
>> if (flags & I915_BO_ALLOC_CONTIGUOUS)
>> place->flags |= TTM_PL_FLAG_CONTIGUOUS;
>> if (offset != I915_BO_INVALID_OFFSET) {
>> - place->fpfn = offset >> PAGE_SHIFT;
>> - place->lpfn = place->fpfn + (size >> PAGE_SHIFT);
>> + GEM_BUG_ON(!safe_conversion(&place->fpfn, offset >> PAGE_SHIFT));
>> + GEM_BUG_ON(!safe_conversion(&place->lpfn, place->fpfn + (size >> PAGE_SHIFT)));
>
> This would be the natural thing to do with BUG_ON/WARN_ON. And I'd like
> it if we could use it like this. But, as I tried to say, GEM_BUG_ON is
> nothing like BUG_ON/WARN_ON, and no code is generated for
> CONFIG_DRM_I915_DEBUG_GEM=n. And our CI will never catch it because it
> always has CONFIG_DRM_I915_DEBUG_GEM=y.
>
Hi Jani,
Thanks for the detailed explanation of what the build option
CONFIG_DRM_I915_DEBUG_GEM doesn't cover.
Using the WARN_ON() macro, I modified with the way in your comments on
v5 version and sent the v7 patch again.
Many thanks,
G.G
> BR,
> Jani.
>
>
>> } else if (mr->io_size && mr->io_size < mr->total) {
>> if (flags & I915_BO_ALLOC_GPU_ONLY) {
>> place->flags |= TTM_PL_FLAG_TOPDOWN;
>> } else {
>> place->fpfn = 0;
>> - place->lpfn = mr->io_size >> PAGE_SHIFT;
>> + GEM_BUG_ON(!safe_conversion(&place->lpfn, mr->io_size >> PAGE_SHIFT));
>> }
>> }
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
>> index 575d67bc6ffe..c480b0b50bcc 100644
>> --- a/drivers/gpu/drm/i915/intel_region_ttm.c
>> +++ b/drivers/gpu/drm/i915/intel_region_ttm.c
>> @@ -209,14 +209,28 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem,
>> if (flags & I915_BO_ALLOC_CONTIGUOUS)
>> place.flags |= TTM_PL_FLAG_CONTIGUOUS;
>> if (offset != I915_BO_INVALID_OFFSET) {
>> - place.fpfn = offset >> PAGE_SHIFT;
>> - place.lpfn = place.fpfn + (size >> PAGE_SHIFT);
>> + if (!safe_conversion(&place.fpfn, offset >> PAGE_SHIFT)) {
>> + GEM_BUG_ON(!safe_conversion(&place.fpfn,offset >> PAGE_SHIFT));
>> + ret = -E2BIG;
>> + goto out;
>> + }
>> + if (!safe_conversion(&place.lpfn, place.fpfn + (size >> PAGE_SHIFT))) {
>> + GEM_BUG_ON(!safe_conversion(&place.lpfn,
>> + place.fpfn + (size >> PAGE_SHIFT)));
>> + ret = -E2BIG;
>> + goto out;
>> + }
>> } else if (mem->io_size && mem->io_size < mem->total) {
>> if (flags & I915_BO_ALLOC_GPU_ONLY) {
>> place.flags |= TTM_PL_FLAG_TOPDOWN;
>> } else {
>> place.fpfn = 0;
>> - place.lpfn = mem->io_size >> PAGE_SHIFT;
>> + if (!safe_conversion(&place.lpfn, mem->io_size >> PAGE_SHIFT)) {
>> + GEM_BUG_ON(!safe_conversion(&place.lpfn,
>> + mem->io_size >> PAGE_SHIFT));
>> + ret = -E2BIG;
>> + goto out;
>> + }
>> }
>> }
>>
>> @@ -224,6 +238,8 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem,
>> mock_bo.bdev = &mem->i915->bdev;
>>
>> ret = man->func->alloc(man, &mock_bo, &place, &res);
>> +
>> +out:
>> if (ret == -ENOSPC)
>> ret = -ENXIO;
>> if (!ret)
>
Powered by blists - more mailing lists