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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.