Am 18.02.20 um 22:46 schrieb Luben Tuikov:
On 2020-02-17 10:08 a.m., Christian König wrote:
Am 17.02.20 um 15:44 schrieb Alex Deucher:
On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov@xxxxxxx> wrote:
Add a AMDGPU_GEM_CREATE_MASK and use it to check
for valid/invalid GEM create flags coming in from
userspace.
Fix a bug in checking whether TMZ is supported at
GEM create time.
Signed-off-by: Luben Tuikov <luben.tuikov@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
include/uapi/drm/amdgpu_drm.h | 2 ++
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index b51a060c637d..74bb79e64fa3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
int r;
/* reject invalid gem flags */
- if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
- AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
- AMDGPU_GEM_CREATE_CPU_GTT_USWC |
- AMDGPU_GEM_CREATE_VRAM_CLEARED |
- AMDGPU_GEM_CREATE_VM_ALWAYS_VALID |
- AMDGPU_GEM_CREATE_EXPLICIT_SYNC |
- AMDGPU_GEM_CREATE_ENCRYPTED))
-
I'd rather keep the list explicit so no one ends up forgetting to
update the mask the next time new flags are added.
Additional to that this patch is bogus.
So the only "additional" thing you're contributing to the review of
this patch is calling it "bogus". Characterizing the patch with an adjective
as "bogus" is very disturbing. What's next?
Well the patch is technical incorrect and breaks the code in a very
subtle and hard to detect manner. Alex didn't noticed that either.
I'm not a native speaker of English, but as far as I have learned
"bogus" is the right adjective for that.
We intentionally filter out the flags here which userspace is allowed to
specify in the GEM interface, but after this patch we would allow all
flags to be specified.
I thought that that could be the case.
Then why did you send out a patch which is seriously broken like that?
I mean if I hadn't noticed it by chance we would have committed this and
added a potentially security problematic bug to the IOCTL interface.
But in your review why not
recommend we have a mask to check user-settable flags? So the
actual flags are collected and visible in one place and well
understood that that is what is being checked and well-defined
by a macro with an appropriate name?
See the flags tested here are the flags currently accepted by the
amdgpu_gem_create_ioctl() function. It doesn't say anything about what
the kernel would accept in the future.
So when we move that into the UAPI header we give userspace a
technically incorrect value.
Why did NO ONE comment on the bug fix below? No one.
Because you mixed up a style change into some bug fix. That people go
for the problematic parts during code review is not really surprising at
all.
Regards,
Christian.
Regards,
Luben
Christian.
Alex
+ if (flags & ~AMDGPU_GEM_CREATE_MASK)
return -EINVAL;
/* reject invalid gem domains */
if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
return -EINVAL;
- if (amdgpu_is_tmz(adev) && (flags & AMDGPU_GEM_CREATE_ENCRYPTED)) {
+ if (!amdgpu_is_tmz(adev) && flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
DRM_ERROR("Cannot allocate secure buffer since TMZ is disabled\n");
return -EINVAL;
}
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index eaf94a421901..c8463cdf4448 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -141,6 +141,8 @@ extern "C" {
*/
#define AMDGPU_GEM_CREATE_ENCRYPTED (1 << 10)
+#define AMDGPU_GEM_CREATE_MASK ((1 << 11)-1)
+
struct drm_amdgpu_gem_create_in {
/** the requested memory size */
__u64 bo_size;
--
2.25.0.232.gd8437c57fa
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cb1fdc3970a224fc61fdc08d7b3bb4613%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175489240077250&sdata=rE8A6jKAIX081ZhxxcMc%2BpGdXvsLUdrAW4AkLsTpNxg%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cb1fdc3970a224fc61fdc08d7b3bb4613%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175489240077250&sdata=rE8A6jKAIX081ZhxxcMc%2BpGdXvsLUdrAW4AkLsTpNxg%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
