From: Steve French <smfrench@gmail.com>
To: Enzo Matsumiya <ematsumiya@suse.de>
Cc: linux-cifs@vger.kernel.org, pc@manguebit.com,
ronniesahlberg@gmail.com, sprasad@microsoft.com, tom@talpey.com,
bharathsm@microsoft.com, henrique.carvalho@suse.com
Subject: Re: [PATCH v2] smb: client: fix crypto buffers in non-linear memory
Date: Thu, 25 Sep 2025 10:46:10 -0500 [thread overview]
Message-ID: <CAH2r5mtNDm9E-vrQjTe6KX=Mo3jLoMShiiTrpN1FwE3b=Nv8LA@mail.gmail.com> (raw)
In-Reply-To: <20250925151033.620865-1-ematsumiya@suse.de>
merged into cifs-2.6.git for-next pending additional testing/review
thx
On Thu, Sep 25, 2025 at 10:10 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> The crypto API, through the scatterlist API, expects input buffers to be
> in linear memory. We handle this with the cifs_sg_set_buf() helper
> that converts vmalloc'd memory to their corresponding pages.
>
> However, when we allocate our aead_request buffer (@creq in
> smb2ops.c::crypt_message()), we do so with kvzalloc(), which possibly
> puts aead_request->__ctx in vmalloc area.
>
> AEAD algorithm then uses ->__ctx for its private/internal data and
> operations, and uses sg_set_buf() for such data on a few places.
>
> This works fine as long as @creq falls into kmalloc zone (small
> requests) or vmalloc'd memory is still within linear range.
>
> Tasks' stacks are vmalloc'd by default (CONFIG_VMAP_STACK=y), so too
> many tasks will increment the base stacks' addresses to a point where
> virt_addr_valid(buf) will fail (BUG() in sg_set_buf()) when that
> happens.
>
> In practice: too many parallel reads and writes on an encrypted mount
> will trigger this bug.
>
> To fix this, always alloc @creq with kmalloc() instead.
> Also drop the @sensitive_size variable/arguments since
> kfree_sensitive() doesn't need it.
>
> Backtrace:
>
> [ 945.272081] ------------[ cut here ]------------
> [ 945.272774] kernel BUG at include/linux/scatterlist.h:209!
> [ 945.273520] Oops: invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC NOPTI
> [ 945.274412] CPU: 7 UID: 0 PID: 56 Comm: kworker/u33:0 Kdump: loaded Not tainted 6.15.0-lku-11779-g8e9d6efccdd7-dirty #1 PREEMPT(voluntary)
> [ 945.275736] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-2-gc13ff2cd-prebuilt.qemu.org 04/01/2014
> [ 945.276877] Workqueue: writeback wb_workfn (flush-cifs-2)
> [ 945.277457] RIP: 0010:crypto_gcm_init_common+0x1f9/0x220
> [ 945.278018] Code: b0 00 00 00 48 83 c4 08 5b 5d 41 5c 41 5d 41 5e 41 5f c3 cc cc cc cc 48 c7 c0 00 00 00 80 48 2b 05 5c 58 e5 00 e9 58 ff ff ff <0f> 0b 0f 0b 0f 0b 0f 0b 0f 0b 0f 0b 48 c7 04 24 01 00 00 00 48 8b
> [ 945.279992] RSP: 0018:ffffc90000a27360 EFLAGS: 00010246
> [ 945.280578] RAX: 0000000000000000 RBX: ffffc90001d85060 RCX: 0000000000000030
> [ 945.281376] RDX: 0000000000080000 RSI: 0000000000000000 RDI: ffffc90081d85070
> [ 945.282145] RBP: ffffc90001d85010 R08: ffffc90001d85000 R09: 0000000000000000
> [ 945.282898] R10: ffffc90001d85090 R11: 0000000000001000 R12: ffffc90001d85070
> [ 945.283656] R13: ffff888113522948 R14: ffffc90001d85060 R15: ffffc90001d85010
> [ 945.284407] FS: 0000000000000000(0000) GS:ffff8882e66cf000(0000) knlGS:0000000000000000
> [ 945.285262] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 945.285884] CR2: 00007fa7ffdd31f4 CR3: 000000010540d000 CR4: 0000000000350ef0
> [ 945.286683] Call Trace:
> [ 945.286952] <TASK>
> [ 945.287184] ? crypt_message+0x33f/0xad0 [cifs]
> [ 945.287719] crypto_gcm_encrypt+0x36/0xe0
> [ 945.288152] crypt_message+0x54a/0xad0 [cifs]
> [ 945.288724] smb3_init_transform_rq+0x277/0x300 [cifs]
> [ 945.289300] smb_send_rqst+0xa3/0x160 [cifs]
> [ 945.289944] cifs_call_async+0x178/0x340 [cifs]
> [ 945.290514] ? __pfx_smb2_writev_callback+0x10/0x10 [cifs]
> [ 945.291177] smb2_async_writev+0x3e3/0x670 [cifs]
> [ 945.291759] ? find_held_lock+0x32/0x90
> [ 945.292212] ? netfs_advance_write+0xf2/0x310
> [ 945.292723] netfs_advance_write+0xf2/0x310
> [ 945.293210] netfs_write_folio+0x346/0xcc0
> [ 945.293689] ? __pfx__raw_spin_unlock_irq+0x10/0x10
> [ 945.294250] netfs_writepages+0x117/0x460
> [ 945.294724] do_writepages+0xbe/0x170
> [ 945.295152] ? find_held_lock+0x32/0x90
> [ 945.295600] ? kvm_sched_clock_read+0x11/0x20
> [ 945.296103] __writeback_single_inode+0x56/0x4b0
> [ 945.296643] writeback_sb_inodes+0x229/0x550
> [ 945.297140] __writeback_inodes_wb+0x4c/0xe0
> [ 945.297642] wb_writeback+0x2f1/0x3f0
> [ 945.298069] wb_workfn+0x300/0x490
> [ 945.298472] process_one_work+0x1fe/0x590
> [ 945.298949] worker_thread+0x1ce/0x3c0
> [ 945.299397] ? __pfx_worker_thread+0x10/0x10
> [ 945.299900] kthread+0x119/0x210
> [ 945.300285] ? __pfx_kthread+0x10/0x10
> [ 945.300729] ret_from_fork+0x119/0x1b0
> [ 945.301163] ? __pfx_kthread+0x10/0x10
> [ 945.301601] ret_from_fork_asm+0x1a/0x30
> [ 945.302055] </TASK>
>
> Fixes: d08089f649a0 ("cifs: Change the I/O paths to use an iterator rather than a page list")
> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
> v2:
> - Change the fix to Paulo's suggestion. Update commit message to reflect it.
>
> fs/smb/client/smb2ops.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index e586f3f4b5c9..68286673afc9 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -4219,7 +4219,7 @@ fill_transform_hdr(struct smb2_transform_hdr *tr_hdr, unsigned int orig_len,
> static void *smb2_aead_req_alloc(struct crypto_aead *tfm, const struct smb_rqst *rqst,
> int num_rqst, const u8 *sig, u8 **iv,
> struct aead_request **req, struct sg_table *sgt,
> - unsigned int *num_sgs, size_t *sensitive_size)
> + unsigned int *num_sgs)
> {
> unsigned int req_size = sizeof(**req) + crypto_aead_reqsize(tfm);
> unsigned int iv_size = crypto_aead_ivsize(tfm);
> @@ -4236,9 +4236,8 @@ static void *smb2_aead_req_alloc(struct crypto_aead *tfm, const struct smb_rqst
> len += req_size;
> len = ALIGN(len, __alignof__(struct scatterlist));
> len += array_size(*num_sgs, sizeof(struct scatterlist));
> - *sensitive_size = len;
>
> - p = kvzalloc(len, GFP_NOFS);
> + p = kzalloc(len, GFP_NOFS);
> if (!p)
> return ERR_PTR(-ENOMEM);
>
> @@ -4252,16 +4251,14 @@ static void *smb2_aead_req_alloc(struct crypto_aead *tfm, const struct smb_rqst
>
> static void *smb2_get_aead_req(struct crypto_aead *tfm, struct smb_rqst *rqst,
> int num_rqst, const u8 *sig, u8 **iv,
> - struct aead_request **req, struct scatterlist **sgl,
> - size_t *sensitive_size)
> + struct aead_request **req, struct scatterlist **sgl)
> {
> struct sg_table sgtable = {};
> unsigned int skip, num_sgs, i, j;
> ssize_t rc;
> void *p;
>
> - p = smb2_aead_req_alloc(tfm, rqst, num_rqst, sig, iv, req, &sgtable,
> - &num_sgs, sensitive_size);
> + p = smb2_aead_req_alloc(tfm, rqst, num_rqst, sig, iv, req, &sgtable, &num_sgs);
> if (IS_ERR(p))
> return ERR_CAST(p);
>
> @@ -4350,7 +4347,6 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
> DECLARE_CRYPTO_WAIT(wait);
> unsigned int crypt_len = le32_to_cpu(tr_hdr->OriginalMessageSize);
> void *creq;
> - size_t sensitive_size;
>
> rc = smb2_get_enc_key(server, le64_to_cpu(tr_hdr->SessionId), enc, key);
> if (rc) {
> @@ -4376,8 +4372,7 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
> return rc;
> }
>
> - creq = smb2_get_aead_req(tfm, rqst, num_rqst, sign, &iv, &req, &sg,
> - &sensitive_size);
> + creq = smb2_get_aead_req(tfm, rqst, num_rqst, sign, &iv, &req, &sg);
> if (IS_ERR(creq))
> return PTR_ERR(creq);
>
> @@ -4407,7 +4402,7 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
> if (!rc && enc)
> memcpy(&tr_hdr->Signature, sign, SMB2_SIGNATURE_SIZE);
>
> - kvfree_sensitive(creq, sensitive_size);
> + kfree_sensitive(creq);
> return rc;
> }
>
> --
> 2.49.0
>
--
Thanks,
Steve
prev parent reply other threads:[~2025-09-25 15:46 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-25 15:10 [PATCH v2] smb: client: fix crypto buffers in non-linear memory Enzo Matsumiya
2025-09-25 15:46 ` Steve French [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAH2r5mtNDm9E-vrQjTe6KX=Mo3jLoMShiiTrpN1FwE3b=Nv8LA@mail.gmail.com' \
--to=smfrench@gmail.com \
--cc=bharathsm@microsoft.com \
--cc=ematsumiya@suse.de \
--cc=henrique.carvalho@suse.com \
--cc=linux-cifs@vger.kernel.org \
--cc=pc@manguebit.com \
--cc=ronniesahlberg@gmail.com \
--cc=sprasad@microsoft.com \
--cc=tom@talpey.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).