linux-kernel - Re: [PATCH] xfs: Fix undefined behavior of shift into sign bit

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]

Date:   Mon, 8 May 2023 08:29:52 -0700
From:   "Darrick J. Wong" <djwong@...nel.org>
To:     Geert Uytterhoeven <geert+renesas@...der.be>
Cc:     Dave Chinner <dchinner@...hat.com>, linux-xfs@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xfs: Fix undefined behavior of shift into sign bit

On Mon, May 08, 2023 at 02:06:34PM +0200, Geert Uytterhoeven wrote:
> With gcc-5:
> 
>     In file included from ./include/trace/define_trace.h:102:0,
> 		     from ./fs/xfs/scrub/trace.h:988,
> 		     from fs/xfs/scrub/trace.c:40:
>     ./fs/xfs/./scrub/trace.h: In function ‘trace_raw_output_xchk_fsgate_class’:
>     ./fs/xfs/scrub/scrub.h:111:28: error: initializer element is not constant
>      #define XREP_ALREADY_FIXED (1 << 31) /* checking our repair work */
> 				^
> 
> Shifting the (signed) value 1 into the sign bit is undefined behavior.
> 
> Fix this for all definitions in the file by shifting "1U" instead of
> "1".
> 
> This was exposed by the first user added in commit 466c525d6d35e691
> ("xfs: minimize overhead of drain wakeups by using jump labels").
> 
> Fixes: 160b5a784525e8a4 ("xfs: hoist the already_fixed variable to the scrub context")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
> ---
>  fs/xfs/scrub/scrub.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
> index b38e93830ddea2b0..e113f2f5c254b085 100644
> --- a/fs/xfs/scrub/scrub.h
> +++ b/fs/xfs/scrub/scrub.h
> @@ -105,10 +105,10 @@ struct xfs_scrub {
>  };
>  
>  /* XCHK state flags grow up from zero, XREP state flags grown down from 2^31 */
> -#define XCHK_TRY_HARDER		(1 << 0)  /* can't get resources, try again */
> -#define XCHK_FSGATES_DRAIN	(1 << 2)  /* defer ops draining enabled */
> -#define XCHK_NEED_DRAIN		(1 << 3)  /* scrub needs to drain defer ops */
> -#define XREP_ALREADY_FIXED	(1 << 31) /* checking our repair work */
> +#define XCHK_TRY_HARDER		(1U << 0)  /* can't get resources, try again */
> +#define XCHK_FSGATES_DRAIN	(1U << 2)  /* defer ops draining enabled */
> +#define XCHK_NEED_DRAIN		(1U << 3)  /* scrub needs to drain defer ops */
> +#define XREP_ALREADY_FIXED	(1U << 31) /* checking our repair work */

DOH.  My mistake. :(

Does gcc have an explicit warning for that?  I turned on W=12e on gcc
11.3 and UBSAN and neither complain about this.

Reviewed-by: Darrick J. Wong <djwong@...nel.org>

--D

>  
>  /*
>   * The XCHK_FSGATES* flags reflect functionality in the main filesystem that
> -- 
> 2.34.1
>