linux-kernel - Re: [PATCH] io_uring/uring_cmd: add missing READ

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9a4261bd-7c8c-4413-b9ac-ae1aeead1060@kernel.dk>
Date: Thu, 23 Jan 2025 08:11:35 -0700
From: Jens Axboe <axboe@...nel.dk>
To: Jann Horn <jannh@...gle.com>
Cc: Pavel Begunkov <asml.silence@...il.com>, io-uring@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] io_uring/uring_cmd: add missing READ_ONCE() on shared
 memory read

On 1/23/25 7:44 AM, Jann Horn wrote:
> On Thu, Jan 23, 2025 at 1:18?AM Jens Axboe <axboe@...nel.dk> wrote:
>> On 1/22/25 12:38 PM, Jens Axboe wrote:
>>>
>>> On Tue, 21 Jan 2025 17:09:59 +0100, Jann Horn wrote:
>>>> cmd->sqe seems to point to shared memory here; so values should only be
>>>> read from it with READ_ONCE(). To ensure that the compiler won't generate
>>>> code that assumes the value in memory will stay constant, add a
>>>> READ_ONCE().
>>>> The callees io_uring_cmd_getsockopt() and io_uring_cmd_setsockopt() already
>>>> do this correctly.
>>>>
>>>> [...]
>>>
>>> Applied, thanks!
>>>
>>> [1/1] io_uring/uring_cmd: add missing READ_ONCE() on shared memory read
>>>       commit: 0963dba3dc006b454c54fd019bbbdb931e7a7c70
>>
>> I took a closer look and this isn't necessary. Either ->sqe is a full
>> copy at this point. Should probably be renamed as such... If we want to
>> make this clearer, then we should do:
> 
> Are you sure? On mainline (at commit 21266b8df522), I applied the
> attached diff that basically adds some printf debugging and adds this
> in io_uring_cmd_sock():

Yeah you are right, braino on my part. If we don't go async, it's not
copied. The changed fix is still better, but I'll reword the commit
message to be more accurate.

-- 
Jens Axboe