linux-kernel - Re: [PATCH] rust: print: avoid evaluating arguments in `pr

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: <Y7yE0pWJeWRbok3V@boqun-archlinux>
Date:   Mon, 9 Jan 2023 13:19:14 -0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Miguel Ojeda <ojeda@...nel.org>
Cc:     Wedson Almeida Filho <wedsonaf@...il.com>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
        patches@...ts.linux.dev,
        Domen Puncer Kugler <domen.puncerkugler@...group.com>
Subject: Re: [PATCH] rust: print: avoid evaluating arguments in `pr_*` macros
 in `unsafe` blocks

On Mon, Jan 09, 2023 at 09:49:12PM +0100, Miguel Ojeda wrote:
> At the moment it is possible to perform unsafe operations in
> the arguments of `pr_*` macros since they are evaluated inside
> an `unsafe` block:
> 
>     let x = &10u32 as *const u32;
>     pr_info!("{}", *x);
> 
> In other words, this is a soundness issue.
> 
> Fix it so that it requires an explicit `unsafe` block.
> 
> Reported-by: Wedson Almeida Filho <wedsonaf@...il.com>
> Reported-by: Domen Puncer Kugler <domen.puncerkugler@...group.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/479
> Signed-off-by: Miguel Ojeda <ojeda@...nel.org>

Reviewed-by: Boqun Feng <boqun.feng@...il.com>

> ---
>  rust/kernel/print.rs | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
> index 29bf9c2e8aee..30103325696d 100644
> --- a/rust/kernel/print.rs
> +++ b/rust/kernel/print.rs
> @@ -142,17 +142,24 @@ pub fn call_printk_cont(args: fmt::Arguments<'_>) {
>  macro_rules! print_macro (
>      // The non-continuation cases (most of them, e.g. `INFO`).
>      ($format_string:path, false, $($arg:tt)+) => (
> -        // SAFETY: This hidden macro should only be called by the documented
> -        // printing macros which ensure the format string is one of the fixed
> -        // ones. All `__LOG_PREFIX`s are null-terminated as they are generated
> -        // by the `module!` proc macro or fixed values defined in a kernel
> -        // crate.
> -        unsafe {
> -            $crate::print::call_printk(
> -                &$format_string,
> -                crate::__LOG_PREFIX,
> -                format_args!($($arg)+),
> -            );
> +        // To remain sound, `arg`s must be expanded outside the `unsafe` block.
> +        // Typically one would use a `let` binding for that; however, `format_args!`
> +        // takes borrows on the arguments, but does not extend the scope of temporaries.
> +        // Therefore, a `match` expression is used to keep them around, since
> +        // the scrutinee is kept until the end of the `match`.
> +        match format_args!($($arg)+) {
> +            // SAFETY: This hidden macro should only be called by the documented
> +            // printing macros which ensure the format string is one of the fixed
> +            // ones. All `__LOG_PREFIX`s are null-terminated as they are generated
> +            // by the `module!` proc macro or fixed values defined in a kernel
> +            // crate.
> +            args => unsafe {
> +                $crate::print::call_printk(
> +                    &$format_string,
> +                    crate::__LOG_PREFIX,
> +                    args,
> +                );
> +            }
>          }
>      );
>  
> 
> base-commit: b7bfaa761d760e72a969d116517eaa12e404c262
> -- 
> 2.39.0
>