Re: [PATCH] drm/ttm: don't set page->mapping

From: "Christian König" <christian.koenig@amd.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Huang Rui <ray.huang@amd.com>, Brian Paul <brianp@vmware.com>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/ttm: don't set page->mapping
Date: Fri, 6 Nov 2020 09:30:19 +0100	[thread overview]
Message-ID: <eb149c81-3c7c-c9db-2c90-4cc2a032c8f4@amd.com> (raw)
In-Reply-To: <CAKMK7uEUuHSMYH5rSbb4c3_fLidbn-fiRGxV+sy6HbhSnWDhtg@mail.gmail.com>

Am 05.11.20 um 17:37 schrieb Daniel Vetter:
> [SNIP]
>>>>>>>>>>> NAK, we use this to determine if a pages belongs to the driver or not in
>>>>>>>>>>> amdgpu for example.
>>>>>>>>>>>
>>>>>>>>>>> Mostly used for debugging, but I would really like to keep that.
>>>>>>>>>> Can you pls point me at that code? A quick grep hasn't really found much at all.
>>>>>>>>> See amdgpu_iomem_read() for an example:
>>>>>>>> Why do you reject this?
>>>>>>> When IOMMU is disabled or uses an 1 to 1 mapping we would otherwise give the
>>>>>>> same access as /dev/mem to system memory and that is forbidden. But as I
>>>>>>> noted this is just for the debugfs file.
>>>>>> Ah, there's a config option for that. Plus it's debugfs, anything goes in
>>>>>> debugfs, but if you're worried about that hole we should just disable the
>>>>>> entire debugfs file for CONFIG_STRICT_DEVMEM. I can perhaps throw that on
>>>>>> top, that follow_pfn patch series I'm baking is all about this kind of
>>>>>> fun.
>>>>> And exactly that would get a NAK from us.
>>>>>
>>>>> We have specially created that debugfs file as an alternative when
>>>>> CONFIG_STRICT_DEVMEM is set.
>>>> Uh that doesn't work if you work around core restrictions with your
>>>> own debugfs paths.
>> That's why we have the restriction to check the mapping of the pages.
>>
>> This way we only expose the memory which was allocated by our driver and
>> don't allow any uncontrolled access to the whole system memory.
>>
>> We have something similar for radeon as well, but there we have a global
>> GART table which we can use for validating stuff.
> The check doesn't take any locks over the check and copy*user, I don't
> think it's protecting anything really against somewhat adverse
> userspace.

You mean that it's racing with freeing the pages? Yes that is an obvious 
problem, but we already had the same issue for radeon as well.

On the other hand we could easily prevent that with a lock.

> I mean fundamentally locking down stuff like STRICT_DEVMEM or all the
> others makes debugging harder, that's kinda the expected tradeoff.

Well we just wanted to have the same debugging functionality we had for 
radeon.

As you said debugfs is a rabbit hole regarding attack vectors anyway.

If you are root you can just go to /sys and start reprogramming the 
hardware to do what you want to do.

Regards,
Christian.

>
>>>>    Maybe you can do fun like this in your dkms, but
>>>> not in upstream. Like if this was specifically created to work around
>>>> CONFIG_STRICT_DEVMEM (and it sounds like that) then I think this
>>>> should never have landed in upstream to begin with.
>>> I'm also kinda confused that there's distros with CONFIG_STRICT_DEVMEM
>>> which allow debugfs. debugfs is a pretty bad root hole all around, or
>>> at least that's been the assumption all the time.
>> Yeah, completely agree :) But that's not my problem.
> I guess I'll do another rfc series and poke a pile of people ... seems
> to be a habit I'm developing :-)
> -Daniel
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel