Message-ID: <20170602180808.GB5636@birch.djwong.org>
Date:   Fri, 2 Jun 2017 11:08:08 -0700
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     David Miller <davem@...emloft.net>
Cc:     herbert@...dor.apana.org.au, linux-crypto@...r.kernel.org,
        sparclinux@...r.kernel.org, linux-btrfs@...r.kernel.org,
        clm@...com, jbacik@...com, dsterba@...e.com, matorola@...il.com,
        sandeen@...deen.net, linux-xfs@...r.kernel.org,
        linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: crypto: Work around deallocated stack frame reference gcc bug on
 sparc.

[add ext4 list to cc]

On Fri, Jun 02, 2017 at 11:28:54AM -0400, David Miller wrote:
> 
> On sparc, if we have an alloca() like situation, as is the case with
> SHASH_DESC_ON_STACK(), we can end up referencing deallocated stack
> memory.  The result can be that the value is clobbered if a trap
> or interrupt arrives at just the right instruction.
> 
> It only occurs if the function ends returning a value from that
> alloca() area and that value can be placed into the return value
> register using a single instruction.
> 
> For example, in lib/libcrc32c.c:crc32c() we end up with a return
> sequence like:
> 
>         return  %i7+8
>          lduw   [%o5+16], %o0   ! MEM[(u32 *)__shash_desc.1_10 + 16B],
> 
> %o5 holds the base of the on-stack area allocated for the shash
> descriptor.  But the return released the stack frame and the
> register window.
> 
> So if an intererupt arrives between 'return' and 'lduw', then
> the value read at %o5+16 can be corrupted.
> 
> Add a data compiler barrier to work around this problem.  This is
> exactly what the gcc fix will end up doing as well, and it absolutely
> should not change the code generated for other cpus (unless gcc
> on them has the same bug :-)
> 
> With crucial insight from Eric Sandeen.
> 
> Reported-by: Anatoly Pugachev <matorola@...il.com>
> Signed-off-by: David S. Miller <davem@...emloft.net>
> ---
> 
> See the thread anchored at:
> 
> 	http://marc.info/?l=linux-sparc&m=149623182616944&w=2
> 
> for discussion, it has a reproducer module.  The problem was
> first noticed as occaisional XFS checksum corruptions.
> 
> Herbert, I don't expect you to like this but it is the best we can do
> I think.  It should not pessimize code on other architectures at all.
> I will work on fixing the gcc bug but it's been around forever and all
> versions are effected.
> 
> I noticed while working on this that at least btrfs duplicates the
> facilities provided by lib/libcrc32c.c and therefore should probably
> be converted over to straight crc32c() calls if possible.

ext4/jbd2's crc32c implementations will also need a fix like this for
{ext4,jbd2}_chksum.  Note that both of these modules call the crypto api
directly to avoid a static dependence on libcrc32c; this was done to
reduce kernel footprint for applications that don't need it.  (ext2,
ext3, and ext4 before the metadata_csum feature existed).

--D

> 
> Thanks!
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
> index ecdba2f..1ac5b85 100644
> --- a/drivers/infiniband/sw/rxe/rxe.h
> +++ b/drivers/infiniband/sw/rxe/rxe.h
> @@ -68,6 +68,7 @@
>  static inline u32 rxe_crc32(struct rxe_dev *rxe,
>  			    u32 crc, void *next, size_t len)
>  {
> +	u32 retval;
>  	int err;
>  
>  	SHASH_DESC_ON_STACK(shash, rxe->tfm);
> @@ -81,7 +82,9 @@ static inline u32 rxe_crc32(struct rxe_dev *rxe,
>  		return crc32_le(crc, next, len);
>  	}
>  
> -	return *(u32 *)shash_desc_ctx(shash);
> +	retval = *(u32 *)shash_desc_ctx(shash);
> +	barrier_data(shash_desc_ctx(shash));
> +	return retval;
>  }
>  
>  int rxe_set_mtu(struct rxe_dev *rxe, unsigned int dev_mtu);
> diff --git a/fs/btrfs/hash.c b/fs/btrfs/hash.c
> index a97fdc1..baacc18 100644
> --- a/fs/btrfs/hash.c
> +++ b/fs/btrfs/hash.c
> @@ -38,6 +38,7 @@ u32 btrfs_crc32c(u32 crc, const void *address, unsigned int length)
>  {
>  	SHASH_DESC_ON_STACK(shash, tfm);
>  	u32 *ctx = (u32 *)shash_desc_ctx(shash);
> +	u32 retval;
>  	int err;
>  
>  	shash->tfm = tfm;
> @@ -47,5 +48,7 @@ u32 btrfs_crc32c(u32 crc, const void *address, unsigned int length)
>  	err = crypto_shash_update(shash, address, length);
>  	BUG_ON(err);
>  
> -	return *ctx;
> +	retval = *ctx;
> +	barrier_data(ctx);
> +	return retval;
>  }
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 2185c7a..fd2e651 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1078,6 +1078,7 @@ static inline u32 f2fs_crc32(struct f2fs_sb_info *sbi, const void *address,
>  {
>  	SHASH_DESC_ON_STACK(shash, sbi->s_chksum_driver);
>  	u32 *ctx = (u32 *)shash_desc_ctx(shash);
> +	u32 retval;
>  	int err;
>  
>  	shash->tfm = sbi->s_chksum_driver;
> @@ -1087,7 +1088,9 @@ static inline u32 f2fs_crc32(struct f2fs_sb_info *sbi, const void *address,
>  	err = crypto_shash_update(shash, address, length);
>  	BUG_ON(err);
>  
> -	return *ctx;
> +	retval = *ctx;
> +	barrier_data(ctx);
> +	return retval;
>  }
>  
>  static inline bool f2fs_crc_valid(struct f2fs_sb_info *sbi, __u32 blk_crc,
> diff --git a/lib/libcrc32c.c b/lib/libcrc32c.c
> index 74a54b7..9f79547 100644
> --- a/lib/libcrc32c.c
> +++ b/lib/libcrc32c.c
> @@ -43,7 +43,7 @@ static struct crypto_shash *tfm;
>  u32 crc32c(u32 crc, const void *address, unsigned int length)
>  {
>  	SHASH_DESC_ON_STACK(shash, tfm);
> -	u32 *ctx = (u32 *)shash_desc_ctx(shash);
> +	u32 ret, *ctx = (u32 *)shash_desc_ctx(shash);
>  	int err;
>  
>  	shash->tfm = tfm;
> @@ -53,7 +53,9 @@ u32 crc32c(u32 crc, const void *address, unsigned int length)
>  	err = crypto_shash_update(shash, address, length);
>  	BUG_ON(err);
>  
> -	return *ctx;
> +	ret = *ctx;
> +	barrier_data(ctx);
> +	return ret;
>  }
>  
>  EXPORT_SYMBOL(crc32c);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists