From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CFB57370D6A for ; Tue, 3 Mar 2026 16:40:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.16 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772556051; cv=none; b=cJ6YknSZcYem3vun5rA5kaZER9mybA8QPJwUZsdf0uCQg1nvCz0nc0iU/tGmRuOvdhfK6EDoHVlLfbbXVXOdNDhrdG+oFbJNC8IIw6ILrKLSGaOCtH5PDl18dy6vN9dTcwe69zTcjP8W4I9k/a7AyH50alq20DLzZNGNn+DOer4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772556051; c=relaxed/simple; bh=LFge7hCh8BNomYatHofqGMkYz9qIzaDpKU3qy29De2g=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=SLEbgvG+aUaGgECUDgTh/6x1JKm1UBx4EnizEaFsAOc4S5IuJpucmce0vY72s+qpyufL4ApluGQwcU7AUQjzNvOomHjDqwXVl4DhnMA3hEFvXQPKh2Ser2NkCMrVKcus+9RXAcFGlyQBZEGRCu27zVD0Tg7d7Ac2dcSOGoiRId4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=DsWdwFir; arc=none smtp.client-ip=192.198.163.16 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="DsWdwFir" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1772556050; x=1804092050; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=LFge7hCh8BNomYatHofqGMkYz9qIzaDpKU3qy29De2g=; b=DsWdwFir4GqgxMCnjloekeADsIMDVfNBEklhpgXWu10DW8zoGLlJgNkJ ScWJwkik7dHVGB9CeFw8pn+XFgkxqFFrPvC1IRIvROXceQ+CVFODcJZwX B/zWW+AEUBA90wWjz8aXKx6EQp/NZMTm+JcCQxA3TZ1N+pkwYIaM1b/y7 8JIByqESGy+NQpZbGbw59Sz9a1laxsoRC1/vzi4jgg/y7aUkiN0kwWXO3 kAaDTemeoomK2O39/MOOIUtzJIWLfLTOju74nBKJ2JnAZrp4RwV6UbKIY ZYKy+QX4BFqaXXlEEdU9zcZ0jJM7vCUye5g6ucWqBkZZXGkCmq1CPlq1b Q==; X-CSE-ConnectionGUID: urj3zXC6Ray0nqjSCJkMEw== X-CSE-MsgGUID: tL0yrHpYRe249YxltOrAjw== X-IronPort-AV: E=McAfee;i="6800,10657,11718"; a="61176212" X-IronPort-AV: E=Sophos;i="6.21,322,1763452800"; d="scan'208";a="61176212" Received: from orviesa002.jf.intel.com ([10.64.159.142]) by fmvoesa110.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Mar 2026 08:40:48 -0800 X-CSE-ConnectionGUID: ztPfAseESfG+sREecl6Bpw== X-CSE-MsgGUID: U8wakuLOQoav51u7TWpitA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.21,322,1763452800"; d="scan'208";a="248543180" Received: from pgcooper-mobl3.ger.corp.intel.com (HELO [10.245.245.25]) ([10.245.245.25]) by orviesa002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Mar 2026 08:40:45 -0800 Message-ID: Date: Tue, 3 Mar 2026 17:40:43 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack To: Julian Orth , =?UTF-8?Q?Michel_D=C3=A4nzer?= Cc: =?UTF-8?Q?Christian_K=C3=B6nig?= , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Dmitry Osipenko , Rob Clark , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20260301-point-v1-1-21fc5fd98614@gmail.com> <3c969254-ed38-4b13-84b3-5afa365b04cb@amd.com> <2b75199f-b78a-4915-8e75-5d186f63f7c5@mailbox.org> Content-Language: en-US From: Maarten Lankhorst In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hey, Den 2026-03-03 kl. 16:36, skrev Julian Orth: > On Tue, Mar 3, 2026 at 4:29 PM Michel Dänzer wrote: >> >> On 3/3/26 15:59, Christian König wrote: >>> On 3/3/26 15:53, Maarten Lankhorst wrote: >>>> Hey, >>>> >>>> Den 2026-03-01 kl. 13:34, skrev Julian Orth: >>>>> Consider the following application: >>>>> >>>>> #include >>>>> #include >>>>> #include >>>>> #include >>>>> >>>>> int main(void) { >>>>> int fd = open("/dev/dri/renderD128", O_RDWR); >>>>> struct drm_syncobj_create arg1; >>>>> ioctl(fd, DRM_IOCTL_SYNCOBJ_CREATE, &arg1); >>>>> struct drm_syncobj_handle arg2; >>>>> memset(&arg2, 1, sizeof(arg2)); // simulate dirty stack >>>>> arg2.handle = arg1.handle; >>>>> arg2.flags = 0; >>>>> arg2.fd = 0; >>>>> arg2.pad = 0; >>>>> // arg2.point = 0; // userspace is required to set point to 0 >>>>> ioctl(fd, DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD, &arg2); >>>>> } >>>>> >>>>> The last ioctl returns EINVAL because args->point is not 0. However, >>>>> userspace developed against older kernel versions is not aware of the >>>>> new point field and might therefore not initialize it. >>>>> >>>>> The correct check would be >>>>> >>>>> if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE) >>>>> return -EINVAL; >>>>> >>>>> However, there might already be userspace that relies on this not >>>>> returning an error as long as point == 0. Therefore use the more lenient >>>>> check. >>>>> >>>>> Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs") >>>>> Signed-off-by: Julian Orth >>>> >>>> I'm not convinced this is the correct fix. >>>> Userspace built before the change had the old size for drm_syncobj_create, >>>> the size is encoded into the ioctl, and zero extended as needed. >>>> >>>> See drivers/gpu/drm/drm_ioctl.c: >>>> out_size = in_size = _IOC_SIZE(cmd); >>>> ... >>>> if (ksize > in_size) >>>> memset(kdata + in_size, 0, ksize - in_size); >>>> >>>> This is a bug in a newly built app, and should be handled by explicitly zeroing >>>> the entire struct or using named initializers, and only setting specific members >>>> as required. >>>> >>>> In particular, apps built before the change will never encounter this bug. >>> >>> Yeah, I've realized that after pushing the patch as well. >>> >>> But I still think this patch is the right thing to do, because without requesting the functionality by setting the flag the point should clearly not have any effect at all. >>> >>> And when an application would have only explicitly assigned the fields known previously and then later been compiled with the new points field it would have failed. >>> >>> It is good practice to memset() structures given to the kernel so that all bytes are zero initialized, but it is not documented as mandatory as far as I know. >> >> Even though it may not be documented, it is in fact mandatory. Otherwise it's not possible to safely extend ioctl structs in general. > > The intention of the original patch was to ignore the args->points > field if the flag is not set: > > if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) > point = args->point; > > Using args->point unconditionally later was therefore a mistake. There is precedence in the ioctl, the pad member is checked against zero for the same reason. The check was there because it is invalid to pass when IMPORT/EXPORT_SYNC_FILE was not set. This is what I would recommend instead: ---- diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 2d4ab745fdad9..176fac24a3198 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -857,7 +857,6 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data, struct drm_syncobj_handle *args = data; unsigned int valid_flags = DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE | DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE; - u64 point = 0; if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) return -EOPNOTSUPP; @@ -868,15 +867,14 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data, if (args->flags & ~valid_flags) return -EINVAL; - if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) - point = args->point; + if (!(args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) && + !(args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE) && + args->point) + return -EINVAL; if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE) return drm_syncobj_export_sync_file(file_private, args->handle, - point, &args->fd); - - if (args->point) - return -EINVAL; + args->point, &args->fd); return drm_syncobj_handle_to_fd(file_private, args->handle, &args->fd); @@ -889,7 +887,6 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, struct drm_syncobj_handle *args = data; unsigned int valid_flags = DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE | DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE; - u64 point = 0; if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) return -EOPNOTSUPP; @@ -900,17 +897,16 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, if (args->flags & ~valid_flags) return -EINVAL; - if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE) - point = args->point; + if (!(args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) && + !(args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE) && + args->point) + return -EINVAL; if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE) return drm_syncobj_import_sync_file_fence(file_private, args->fd, args->handle, - point); - - if (args->point) - return -EINVAL; + args->point); return drm_syncobj_fd_to_handle(file_private, args->fd, &args->handle);