From: Christopher Snowhill <kode54@gmail.com>
To: "Souza, Jose" <jose.souza@intel.com>,
"intel-xe@lists.freedesktop.org" <intel-xe@lists.freedesktop.org>
Subject: Re: [Intel-xe] [PATCH 2/2] drm/xe: Validate uAPI padding and reserved fields
Date: Wed, 24 May 2023 18:40:25 -0700 [thread overview]
Message-ID: <cf961372-f4ce-e489-e5b2-986e111faa0d@gmail.com> (raw)
In-Reply-To: <983c4693da199821c824c90e6a32b6b5770ba3c5.camel@intel.com>
On 5/24/23 08:37, Souza, Jose wrote:
> On Tue, 2023-05-23 at 20:31 -0700, Christopher Snowhill wrote:
>> Padding and reserved fields are declared such that they must be
>> zeroed, so verify that they're all zero in the respective ioctl
>> functions.
>>
>> Derived from original patch by mlankhorst.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Christopher Snowhill <kode54@gmail.com>
>> ---
>> drivers/gpu/drm/xe/xe_bo.c | 6 ++++--
>> drivers/gpu/drm/xe/xe_engine.c | 19 +++++++++++++++----
>> drivers/gpu/drm/xe/xe_exec.c | 4 +++-
>> drivers/gpu/drm/xe/xe_mmio.c | 3 ++-
>> drivers/gpu/drm/xe/xe_query.c | 3 ++-
>> drivers/gpu/drm/xe/xe_sync.c | 4 +++-
>> drivers/gpu/drm/xe/xe_vm.c | 22 +++++++++++++++++++---
>> drivers/gpu/drm/xe/xe_vm_madvise.c | 4 +++-
>> drivers/gpu/drm/xe/xe_wait_user_fence.c | 3 ++-
>> 9 files changed, 53 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index c82e995df779..de713348ccc1 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -1644,7 +1644,8 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
>> u32 handle;
>> int err;
>>
>> - if (XE_IOCTL_ERR(xe, args->extensions))
>> + if (XE_IOCTL_ERR(xe, args->extensions) || XE_IOCTL_ERR(xe, args->pad) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> return -EINVAL;
>>
>> if (XE_IOCTL_ERR(xe, args->flags &
>> @@ -1714,7 +1715,8 @@ int xe_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
>> struct drm_xe_gem_mmap_offset *args = data;
>> struct drm_gem_object *gem_obj;
>>
>> - if (XE_IOCTL_ERR(xe, args->extensions))
>> + if (XE_IOCTL_ERR(xe, args->extensions) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> return -EINVAL;
>>
>> if (XE_IOCTL_ERR(xe, args->flags))
>> diff --git a/drivers/gpu/drm/xe/xe_engine.c b/drivers/gpu/drm/xe/xe_engine.c
>> index 094ec17d3004..8c3a722d91c6 100644
>> --- a/drivers/gpu/drm/xe/xe_engine.c
>> +++ b/drivers/gpu/drm/xe/xe_engine.c
>> @@ -348,7 +348,8 @@ static int engine_user_ext_set_property(struct xe_device *xe,
>> return -EFAULT;
>>
>> if (XE_IOCTL_ERR(xe, ext.property >=
>> - ARRAY_SIZE(engine_set_property_funcs)))
>> + ARRAY_SIZE(engine_set_property_funcs)) ||
>> + XE_IOCTL_ERR(xe, ext.pad))
>> return -EINVAL;
>>
>> idx = array_index_nospec(ext.property, ARRAY_SIZE(engine_set_property_funcs));
>> @@ -380,7 +381,8 @@ static int engine_user_extensions(struct xe_device *xe, struct xe_engine *e,
>> if (XE_IOCTL_ERR(xe, err))
>> return -EFAULT;
>>
>> - if (XE_IOCTL_ERR(xe, ext.name >=
>> + if (XE_IOCTL_ERR(xe, ext.pad) ||
>> + XE_IOCTL_ERR(xe, ext.name >=
>> ARRAY_SIZE(engine_user_extension_funcs)))
>> return -EINVAL;
>>
>> @@ -523,7 +525,8 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
>> int len;
>> int err;
>>
>> - if (XE_IOCTL_ERR(xe, args->flags))
>> + if (XE_IOCTL_ERR(xe, args->flags) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> return -EINVAL;
>>
>> len = args->width * args->num_placements;
>> @@ -639,6 +642,10 @@ int xe_engine_get_property_ioctl(struct drm_device *dev, void *data,
>> struct drm_xe_engine_get_property *args = data;
>> struct xe_engine *e;
>>
>> + if (XE_IOCTL_ERR(xe, args->extensions) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> + return -EINVAL;
> I believe extensions are allowed to be != 0 even if there is no extension yet for this ioctl.
> Also commit message don't say anything about checking extensions, so for now please drop those checks in here and in all other places added in this
> patch.
What about sections of code that already check extensions? Drop
extensions from those when checking the others, or add onto it?
> Other than this LGTM.
I'll send a revision removing extensions, as long as I know what I'm
supposed to do for existing checks which already validated that
extensions was equal to zero.
>
>> +
>> mutex_lock(&xef->engine.lock);
>> e = xa_load(&xef->engine.xa, args->engine_id);
>> mutex_unlock(&xef->engine.lock);
>> @@ -718,7 +725,8 @@ int xe_engine_destroy_ioctl(struct drm_device *dev, void *data,
>> struct drm_xe_engine_destroy *args = data;
>> struct xe_engine *e;
>>
>> - if (XE_IOCTL_ERR(xe, args->pad))
>> + if (XE_IOCTL_ERR(xe, args->pad) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> return -EINVAL;
>>
>> mutex_lock(&xef->engine.lock);
>> @@ -748,6 +756,9 @@ int xe_engine_set_property_ioctl(struct drm_device *dev, void *data,
>> int ret;
>> u32 idx;
>>
>> + if (XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> + return -EINVAL;
>> +
>> e = xe_engine_lookup(xef, args->engine_id);
>> if (XE_IOCTL_ERR(xe, !e))
>> return -ENOENT;
>> diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
>> index 3db1b159586e..e44076ee2e11 100644
>> --- a/drivers/gpu/drm/xe/xe_exec.c
>> +++ b/drivers/gpu/drm/xe/xe_exec.c
>> @@ -181,7 +181,9 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>> bool write_locked;
>> int err = 0;
>>
>> - if (XE_IOCTL_ERR(xe, args->extensions))
>> + if (XE_IOCTL_ERR(xe, args->extensions) ||
>> + XE_IOCTL_ERR(xe, args->pad[0] || args->pad[1] || args->pad[2]) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> return -EINVAL;
>>
>> engine = xe_engine_lookup(xef, args->engine_id);
>> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>> index c7fbb1cc1f64..9d583f11e290 100644
>> --- a/drivers/gpu/drm/xe/xe_mmio.c
>> +++ b/drivers/gpu/drm/xe/xe_mmio.c
>> @@ -407,7 +407,8 @@ int xe_mmio_ioctl(struct drm_device *dev, void *data,
>> bool allowed;
>> int ret = 0;
>>
>> - if (XE_IOCTL_ERR(xe, args->extensions))
>> + if (XE_IOCTL_ERR(xe, args->extensions) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> return -EINVAL;
>>
>> if (XE_IOCTL_ERR(xe, args->flags & ~VALID_MMIO_FLAGS))
>> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
>> index dd64ff0d2a57..97742d003c8a 100644
>> --- a/drivers/gpu/drm/xe/xe_query.c
>> +++ b/drivers/gpu/drm/xe/xe_query.c
>> @@ -374,7 +374,8 @@ int xe_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>> struct drm_xe_device_query *query = data;
>> u32 idx;
>>
>> - if (XE_IOCTL_ERR(xe, query->extensions != 0))
>> + if (XE_IOCTL_ERR(xe, query->extensions ||
>> + XE_IOCTL_ERR(xe, query->reserved[0] || query->reserved[1])))
>> return -EINVAL;
>>
>> if (XE_IOCTL_ERR(xe, query->query > ARRAY_SIZE(xe_query_funcs)))
>> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
>> index 1e4e4acb2c4a..5acb37a8b2ab 100644
>> --- a/drivers/gpu/drm/xe/xe_sync.c
>> +++ b/drivers/gpu/drm/xe/xe_sync.c
>> @@ -111,7 +111,9 @@ int xe_sync_entry_parse(struct xe_device *xe, struct xe_file *xef,
>> return -EFAULT;
>>
>> if (XE_IOCTL_ERR(xe, sync_in.flags &
>> - ~(SYNC_FLAGS_TYPE_MASK | DRM_XE_SYNC_SIGNAL)))
>> + ~(SYNC_FLAGS_TYPE_MASK | DRM_XE_SYNC_SIGNAL)) ||
>> + XE_IOCTL_ERR(xe, sync_in.pad) ||
>> + XE_IOCTL_ERR(xe, sync_in.reserved[0] || sync_in.reserved[1]))
>> return -EINVAL;
>>
>> signal = sync_in.flags & DRM_XE_SYNC_SIGNAL;
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index a0306526b269..ea354ffbede0 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -1799,7 +1799,9 @@ static int vm_user_ext_set_property(struct xe_device *xe, struct xe_vm *vm,
>> return -EFAULT;
>>
>> if (XE_IOCTL_ERR(xe, ext.property >=
>> - ARRAY_SIZE(vm_set_property_funcs)))
>> + ARRAY_SIZE(vm_set_property_funcs)) ||
>> + XE_IOCTL_ERR(xe, ext.pad) ||
>> + XE_IOCTL_ERR(xe, ext.reserved[0] || ext.reserved[1]))
>> return -EINVAL;
>>
>> return vm_set_property_funcs[ext.property](xe, vm, ext.value);
>> @@ -1827,7 +1829,8 @@ static int vm_user_extensions(struct xe_device *xe, struct xe_vm *vm,
>> if (XE_IOCTL_ERR(xe, err))
>> return -EFAULT;
>>
>> - if (XE_IOCTL_ERR(xe, ext.name >=
>> + if (XE_IOCTL_ERR(xe, ext.pad) ||
>> + XE_IOCTL_ERR(xe, ext.name >=
>> ARRAY_SIZE(vm_user_extension_funcs)))
>> return -EINVAL;
>>
>> @@ -1858,6 +1861,9 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
>> int err;
>> u32 flags = 0;
>>
>> + if (XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> + return -EINVAL;
>> +
>> if (XE_IOCTL_ERR(xe, args->flags & ~ALL_DRM_XE_VM_CREATE_FLAGS))
>> return -EINVAL;
>>
>> @@ -1941,7 +1947,8 @@ int xe_vm_destroy_ioctl(struct drm_device *dev, void *data,
>> struct drm_xe_vm_destroy *args = data;
>> struct xe_vm *vm;
>>
>> - if (XE_IOCTL_ERR(xe, args->pad))
>> + if (XE_IOCTL_ERR(xe, args->pad) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> return -EINVAL;
>>
>> vm = xe_vm_lookup(xef, args->vm_id);
>> @@ -2891,6 +2898,8 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe,
>> int i;
>>
>> if (XE_IOCTL_ERR(xe, args->extensions) ||
>> + XE_IOCTL_ERR(xe, args->pad || args->pad2) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]) ||
>> XE_IOCTL_ERR(xe, !args->num_binds) ||
>> XE_IOCTL_ERR(xe, args->num_binds > MAX_BINDS))
>> return -EINVAL;
>> @@ -2923,6 +2932,13 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe,
>> u64 obj_offset = (*bind_ops)[i].obj_offset;
>> u32 region = (*bind_ops)[i].region;
>>
>> + if (XE_IOCTL_ERR(xe, (*bind_ops)[i].pad) ||
>> + XE_IOCTL_ERR(xe, (*bind_ops)[i].reserved[0] ||
>> + (*bind_ops)[i].reserved[1])) {
>> + err = -EINVAL;
>> + goto free_bind_ops;
>> + }
>> +
>> if (i == 0) {
>> *async = !!(op & XE_VM_BIND_FLAG_ASYNC);
>> } else if (XE_IOCTL_ERR(xe, !*async) ||
>> diff --git a/drivers/gpu/drm/xe/xe_vm_madvise.c b/drivers/gpu/drm/xe/xe_vm_madvise.c
>> index 29815852985a..0f5eef337037 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_madvise.c
>> +++ b/drivers/gpu/drm/xe/xe_vm_madvise.c
>> @@ -301,7 +301,9 @@ int xe_vm_madvise_ioctl(struct drm_device *dev, void *data,
>> struct xe_vma **vmas = NULL;
>> int num_vmas = 0, err = 0, idx;
>>
>> - if (XE_IOCTL_ERR(xe, args->extensions))
>> + if (XE_IOCTL_ERR(xe, args->extensions) ||
>> + XE_IOCTL_ERR(xe, args->pad || args->pad2) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> return -EINVAL;
>>
>> if (XE_IOCTL_ERR(xe, args->property > ARRAY_SIZE(madvise_funcs)))
>> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>> index 15c2e5aa08d2..6c8a60c60087 100644
>> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
>> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>> @@ -100,7 +100,8 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>> args->flags & DRM_XE_UFENCE_WAIT_VM_ERROR;
>> unsigned long timeout = args->timeout;
>>
>> - if (XE_IOCTL_ERR(xe, args->extensions))
>> + if (XE_IOCTL_ERR(xe, args->extensions) || XE_IOCTL_ERR(xe, args->pad) ||
>> + XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>> return -EINVAL;
>>
>> if (XE_IOCTL_ERR(xe, args->flags & ~VALID_FLAGS))
next prev parent reply other threads:[~2023-05-25 1:40 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-24 3:31 [Intel-xe] [PATCH 0/2] Update Xe uAPI in a minimally invasive way Christopher Snowhill
2023-05-24 3:31 ` [Intel-xe] [PATCH 1/2] drm/xe: Add explicit padding to uAPI definition Christopher Snowhill
2023-05-24 15:20 ` Souza, Jose
2023-05-24 16:21 ` Lucas De Marchi
2023-05-24 3:31 ` [Intel-xe] [PATCH 2/2] drm/xe: Validate uAPI padding and reserved fields Christopher Snowhill
2023-05-24 15:37 ` Souza, Jose
2023-05-25 1:40 ` Christopher Snowhill [this message]
2023-05-25 13:06 ` Souza, Jose
2023-05-24 23:43 ` Lucas De Marchi
2023-05-24 3:34 ` [Intel-xe] ✓ CI.Patch_applied: success for Update Xe uAPI in a minimally invasive way Patchwork
2023-05-24 3:36 ` [Intel-xe] ✓ CI.KUnit: " Patchwork
2023-05-24 3:40 ` [Intel-xe] ✓ CI.Build: " Patchwork
2023-05-24 4:08 ` [Intel-xe] ○ CI.BAT: info " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cf961372-f4ce-e489-e5b2-986e111faa0d@gmail.com \
--to=kode54@gmail.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=jose.souza@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox