| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e793343b-b97d-3eb8-6c19-bf5ba4074408@amd.com>
Date: Mon, 31 Jul 2023 11:50:53 +0200
From: Christian König <christian.koenig@....com>
To: Boris Brezillon <boris.brezillon@...labora.com>,
Nathan Chancellor <nathan@...nel.org>
Cc: ndesaulniers@...gle.com,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>, Tom Rix <trix@...hat.com>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
llvm@...ts.linux.dev, Naresh Kamboju <naresh.kamboju@...aro.org>,
dakr@...hat.com, alexander.deucher@....com
Subject: Re: [PATCH v2] drm: fix indirect goto into statement expression UB
Am 31.07.23 um 09:03 schrieb Boris Brezillon:
> On Fri, 28 Jul 2023 10:17:57 -0700
> Nathan Chancellor <nathan@...nel.org> wrote:
>
>> + people from trailers of 09593216bff1
>>
>> On Thu, Jul 27, 2023 at 03:50:58PM -0700, ndesaulniers@...gle.com wrote:
>>> A new diagnostic in clang-17 now produces the following build error:
>>>
>>> drivers/gpu/drm/tests/drm_exec_test.c:41:3: error: cannot jump from this
>>> indirect goto statement to one of its possible targets
>>> 41 | drm_exec_retry_on_contention(&exec);
>>> | ^
>>> include/drm/drm_exec.h:96:4: note: expanded from macro
>>> 'drm_exec_retry_on_contention'
>>> 96 | goto *__drm_exec_retry_ptr;
>>> | ^
>>> drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: possible target of
>>> indirect goto statement
>>> 39 | drm_exec_until_all_locked(&exec) {
>>> | ^
>>> include/drm/drm_exec.h:79:33: note: expanded from macro
>>> 'drm_exec_until_all_locked'
>>> 79 | __label__ __drm_exec_retry;
>>> drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: jump enters a
>>> statement expression
>>>
>>> The GCC manually currently states that:
>> ^ manual
>>
>>>>> Jumping into a statement expression with a computed goto (see Labels
>>>>> as Values) has undefined behavior.
>>> So the diagnostic appears correct, even if codegen happened to produce
>>> working code.
>>>
>>> Looking closer at this code, while the original combination of statement
>>> expression, local label, and computed/indirect goto GNU C expressions
>>> were clever, a simple while loop and continue block might have sufficed.
>>>
>>> This approach might not work as expected if drm_exec_until_all_locked
>>> "loops" can be nested, but that doesn't appear to be an existing use
>>> case in the codebase.
> Hm, that's exactly the sort of things we were trying to be robust
> against with the original approach. With this version, we're back to a
> situation where
>
> drm_exec_until_all_locked(exec) {
> for (...) {
> drm_exec_retry_on_contention(exec);
> }
> }
>
> doesn't do what we expect it to do, and that's a use case we want to
> support.
Yeah, agree that isn't what's supposed to happen here and would break a
couple of use cases.
As a workaround we could define the label before the loop, but that
makes the label local to the enclosing block, e.g. allows for using
drm_exec_retry_on_contention() outside of drm_exec_until_all_locked().
Going to work on a patch, thanks for the notice.
Regards,
Christian.
>
>>> Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7")
>>> Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
>>> Link: https://github.com/ClangBuiltLinux/linux/issues/1890
>>> Link: https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
>>> Reported-by: Nathan Chancellor <nathan@...nel.org>
>>> Reported-by: Naresh Kamboju <naresh.kamboju@...aro.org>
>>> Signed-off-by: Nick Desaulniers <ndesaulniers@...gle.com>
>> Thanks for the patch!
>>
>> Tested-by: Nathan Chancellor <nathan@...nel.org> # build
>>
>>> ---
>>> Changes in v2:
>>> Fix the continue to be outside of the do while
>>> - Link to v1: https://lore.kernel.org/r/20230727-amdgpu-v1-1-a95690e75388@google.com
>>> ---
>>> include/drm/drm_exec.h | 21 +++++----------------
>>> 1 file changed, 5 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
>>> index 73205afec162..fa1cc5c3d021 100644
>>> --- a/include/drm/drm_exec.h
>>> +++ b/include/drm/drm_exec.h
>>> @@ -70,18 +70,8 @@ struct drm_exec {
>>> * Core functionality of the drm_exec object. Loops until all GEM objects are
>>> * locked and no more contention exists. At the beginning of the loop it is
>>> * guaranteed that no GEM object is locked.
>>> - *
>>> - * Since labels can't be defined local to the loops body we use a jump pointer
>>> - * to make sure that the retry is only used from within the loops body.
>>> */
>>> -#define drm_exec_until_all_locked(exec) \
>>> - for (void *__drm_exec_retry_ptr; ({ \
>>> - __label__ __drm_exec_retry; \
>>> -__drm_exec_retry: \
>>> - __drm_exec_retry_ptr = &&__drm_exec_retry; \
>>> - (void)__drm_exec_retry_ptr; \
>>> - drm_exec_cleanup(exec); \
>>> - });)
>>> +#define drm_exec_until_all_locked(exec) while(drm_exec_cleanup(exec))
>>>
>>> /**
>>> * drm_exec_retry_on_contention - restart the loop to grap all locks
>>> @@ -90,11 +80,10 @@ __drm_exec_retry: \
>>> * Control flow helper to continue when a contention was detected and we need to
>>> * clean up and re-start the loop to prepare all GEM objects.
>>> */
>>> -#define drm_exec_retry_on_contention(exec) \
>>> - do { \
>>> - if (unlikely(drm_exec_is_contended(exec))) \
>>> - goto *__drm_exec_retry_ptr; \
>>> - } while (0)
>>> +#define drm_exec_retry_on_contention(exec) \
>>> + if (unlikely(drm_exec_is_contended(exec))) \
>>> + continue; \
>>> + do {} while (0)
>>>
>>> /**
>>> * drm_exec_is_contended - check for contention
>>>
>>> ---
>>> base-commit: 451cc82bd11eb6a374f4dbcfc1cf007eafea91ab
>>> change-id: 20230727-amdgpu-93c0e5302951
>>>
>>> Best regards,
>>> --
>>> Nick Desaulniers <ndesaulniers@...gle.com>
>>>
Powered by blists - more mailing lists