[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@xxxxxxxxx> > Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx> > --- > > 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html
