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] [thread-next>] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ