| 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: <d41d123e-d682-4685-88f5-e45567cc1975@proton.me>
Date: Sun, 31 Mar 2024 10:27:37 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Wedson Almeida Filho <wedsonaf@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, Martin Rodriguez Reboredo <yakoyoku@...il.com>, Asahi Lina <lina@...hilina.net>, Eric Curtin <ecurtin@...hat.com>, Neal Gompa <neal@...pa.dev>, Thomas Bertschinger <tahbertschinger@...il.com>, Andrea Righi <andrea.righi@...onical.com>, Sumera Priyadarsini <sylphrenadin@...il.com>, Finn Behrens <me@...enk.dev>, Adam Bratschi-Kaye <ark.email@...il.com>, stable@...r.kernel.org, Daniel Xu <dxu@...uu.xyz>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: macros: fix soundness issue in `module!` macro
On 31.03.24 03:00, Wedson Almeida Filho wrote:
> On Wed, 27 Mar 2024 at 13:04, Benno Lossin <benno.lossin@...ton.me> wrote:
>> + #[cfg(not(MODULE))]
>> + #[doc(hidden)]
>> + #[no_mangle]
>> + pub extern \"C\" fn __{name}_exit() {{
>> + __exit()
I just noticed this should be wrapped in an `unsafe` block with a SAFETY
comment. Will fix this in v2.
>> + }}
>>
>> - #[cfg(not(MODULE))]
>> - #[doc(hidden)]
>> - #[no_mangle]
>> - pub extern \"C\" fn __{name}_exit() {{
>> - __exit()
>> - }}
>> + /// # Safety
>> + ///
>> + /// This function must
>> + /// - only be called once,
>> + /// - not be called concurrently with `__exit`.
>
> I don't think the second item is needed here, it really is a
> requirement on `__exit`.
Fixed.
>
>> + unsafe fn __init() -> core::ffi::c_int {{
>> + match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
>> + Ok(m) => {{
>> + // SAFETY:
>> + // no data race, since `__MOD` can only be accessed by this module and
>> + // there only `__init` and `__exit` access it. These functions are only
>> + // called once and `__exit` cannot be called before or during `__init`.
>> + unsafe {{
>> + __MOD = Some(m);
>> + }}
>> + return 0;
>> + }}
>> + Err(e) => {{
>> + return e.to_errno();
>> + }}
>> + }}
>> + }}
>>
>> - fn __init() -> core::ffi::c_int {{
>> - match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
>> - Ok(m) => {{
>> + /// # Safety
>> + ///
>> + /// This function must
>> + /// - only be called once,
>> + /// - be called after `__init`,
>> + /// - not be called concurrently with `__init`.
>
> The second item is incomplete: it must be called after `__init` *succeeds*.
Indeed.
>
> With that added (which is a different precondition), I think the third
> item can be dropped because if you have to wait to see whether
> `__init` succeeded or failed before you can call `__exit`, then
> certainly you cannot call it concurrently with `__init`.
I would love to drop that requirement, but I am not sure we can. With
that requirement, I wanted to ensure that no data race on `__MOD` can
happen. If you need to verify that `__init` succeeded, one might think
that it is not possible to call `__exit` such that a data race occurs,
but I think it could theoretically be done if the concrete `Module`
implementation never failed.
Do you have any suggestion for what I could add to the "be called after
`__init` was called and returned `0`" requirement to make a data race
impossible?
--
Cheers,
Benno
>
>> + unsafe fn __exit() {{
>> + // SAFETY:
>> + // no data race, since `__MOD` can only be accessed by this module and there
>> + // only `__init` and `__exit` access it. These functions are only called once
>> + // and `__init` was already called.
>> unsafe {{
>> - __MOD = Some(m);
>> + // Invokes `drop()` on `__MOD`, which should be used for cleanup.
>> + __MOD = None;
>> }}
>> - return 0;
>> }}
>> - Err(e) => {{
>> - return e.to_errno();
>> - }}
>> - }}
>> - }}
>>
>> - fn __exit() {{
>> - unsafe {{
>> - // Invokes `drop()` on `__MOD`, which should be used for cleanup.
>> - __MOD = None;
>> + {modinfo}
>> }}
>> }}
>> -
>> - {modinfo}
>> ",
>> type_ = info.type_,
>> name = info.name,
>>
>> base-commit: 4cece764965020c22cff7665b18a012006359095
>> --
>> 2.44.0
>>
>>
Powered by blists - more mailing lists