Re: [PATCH] drm/shmem-helper: Fix BUG_ON() on mmap(PROT_WRITE, MAP_PRIVATE)

[Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>]

Hi,

On 21.05.2024 14:58, Daniel Vetter wrote:
> On Tue, 21 May 2024 at 14:38, Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Mon, May 20, 2024 at 12:05:14PM +0200, Jacek Lawrynowicz wrote:
>>> From: "Wachowski, Karol" <karol.wachowski@intel.com>
>>>
>>> Lack of check for copy-on-write (COW) mapping in drm_gem_shmem_mmap
>>> allows users to call mmap with PROT_WRITE and MAP_PRIVATE flag
>>> causing a kernel panic due to BUG_ON in vmf_insert_pfn_prot:
>>> BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));
>>>
>>> Return -EINVAL early if COW mapping is detected.
>>>
>>> This bug affects all drm drivers using default shmem helpers.
>>> It can be reproduced by this simple example:
>>> void *ptr = mmap(0, size, PROT_WRITE, MAP_PRIVATE, fd, mmap_offset);
>>> ptr[0] = 0;
>>>
>>> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects")
>>> Cc: Noralf Trønnes <noralf@tronnes.org>
>>> Cc: Eric Anholt <eric@anholt.net>
>>> Cc: Rob Herring <robh@kernel.org>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Maxime Ripard <mripard@kernel.org>
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: David Airlie <airlied@gmail.com>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: <stable@vger.kernel.org> # v5.2+
>>> Signed-off-by: Wachowski, Karol <karol.wachowski@intel.com>
>>> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
>>
>> Excellent catch!
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> I reviewed the other helpers, and ttm/vram helpers already block this with
>> the check in ttm_bo_mmap_obj.
>>
>> But the dma helpers does not, because the remap_pfn_range that underlies
>> the various dma_mmap* function (at least on most platforms) allows some
>> limited use of cow. But it makes no sense at all to all that only for
>> gpu buffer objects backed by specific allocators.
>>
>> Would you be up for the 2nd patch that also adds this check to
>> drm_gem_dma_mmap, so that we have a consistent uapi?
>>
>> I'll go ahead and apply this one to drm-misc-fixes meanwhile.
> 
> Forgot to add: A testcase in igt would also be really lovely.
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#validating-changes-with-igt
> -Sima

OK, we will take a look at the test case.
We have no easy way to test dma helpers, so it would be best if someone using them could make a fix.


Regards,
Jacek