On 2020-02-19 3:20 a.m., Christian König wrote: > 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. I wouldn't have expected a petulant-childlike response. I would've expected a leadership-like position, something like: Split your patch into two. The second part is okay with the addition of DRM_PRINT_ONCE, and the first part admits bits which we don't want user space to control. Regards, Luben > > 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