linux-kernel - Re: [PATCH] crypto: arm64/sm4 - Fix possible crash in GCM cryption

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]

Date:   Mon, 30 Jan 2023 16:15:33 +0800
From:   Herbert Xu <herbert@...dor.apana.org.au>
To:     Tianjia Zhang <tianjia.zhang@...ux.alibaba.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, linux-crypto@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Ard Biesheuvel <ardb@...nel.org>
Subject: Re: [PATCH] crypto: arm64/sm4 - Fix possible crash in GCM cryption

On Mon, Jan 30, 2023 at 03:34:42PM +0800, Tianjia Zhang wrote:
>
> I printed the walk->nbytes of each iteration of the walker, it is not
> always multiples of chunksize except at the end when the algorithm test
> manager is turned on.

Sorry I was mistaken.  We only guarantee that a minimum of chunksize
bytes is given to you until the very end, not that it is exactly a
multiple of chunksize.

While you still need to compute tail, you could get rid of the else if
check as walk->nbytes - tail cannot be zero (we must provide you with
at least one chunk before the end):

		if (walk->nbytes == walk->total) {
			tail = 0;

			sm4_ce_pmull_gcm_crypt(ctx->key.rkey_enc, dst, src, iv,
					       walk->nbytes, ghash,
					       ctx->ghash_table,
					       (const u8 *)&lengths);
		} else {
			sm4_ce_pmull_gcm_crypt(ctx->key.rkey_enc, dst, src, iv,
					       walk->nbytes - tail, ghash,
					       ctx->ghash_table, NULL);
		}

In fact we could rewrite it like this:

		unsigned int tail = walk->nbytes % SM4_BLOCK_SIZE;
		unsigned int nbytes = walk->nbytes - tail;
		const u8 *src = walk->src.virt.addr;
		u8 *dst = walk->dst.virt.addr;
		u8 *lp = NULL;

		if (walk->nbytes == walk->total) {
			nbytes = walk->nbytes;
			tail = 0;
			lp = (u8 *)&lengths;
		}

		sm4_ce_pmull_gcm_crypt(ctx->key.rkey_enc, dst, src, iv,
				       nbytes, ghash, ctx->ghash_table, lp);

The second part of that loop could also be rewritten as:

		kernel_neon_end();

		err = skcipher_walk_done(walk, tail);
		if (!walk->nbytes)
			return err;

		kernel_neon_begin();
	} while (1);

Actually I think there is a serious bug here.  If you're doing an
empty message, you must not call skcipher_walk_done as that may
then free random uninitialised stack memory.

Did you copy this code from somewhere else? If so wherever you got
it from needs to be fixed too.  The loop should look like this:

	if (!walk->nbytes) {
		/* iv may be unaligned as the walker didn't run at all. */
		sm4_ce_pmull_gcm_crypt(ctx->key.rkey_enc, NULL, NULL, iv,
				       0, ghash, ctx->ghash_table,
				       (u8 *)&lengths);
		kernel_neon_end();
		return 0;
	}

	do {
		...
	}

Thanks,
-- 
Email: Herbert Xu <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt