From: Tyler Hicks <code@tyhicks.com>
To: Yichong Chen <chenyichong@uniontech.com>
Cc: Thorsten Blum <thorsten.blum@linux.dev>,
Eric Biggers <ebiggers@kernel.org>, Kees Cook <kees@kernel.org>,
ecryptfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ecryptfs: validate packet parser buffer lengths
Date: Mon, 13 Jul 2026 23:19:03 -0500 [thread overview]
Message-ID: <alW4t1KssnzQbl4Y@elm> (raw)
In-Reply-To: <20260627090208.27774-1-chenyichong@uniontech.com>
On 2026-06-27 17:02:08, Yichong Chen wrote:
> ecryptfs_parse_packet_set() receives a pointer into the file header,
> but it calculates the remaining packet buffer size from PAGE_SIZE - 8.
> For version 1 headers the packet set starts later in the header, so
> this can overstate the available buffer.
>
> Pass the actual packet set buffer length from the caller and use the
> remaining length after each parsed packet. Also fix the tag 11 packet
> exact-fit bounds check and reject too-small tag 70 packet bodies before
> subtracting fixed metadata sizes.
My apologies for just now getting to the review of this patch but could
you please split out these two separate fixes (tag 11 exact-fit check
and too-small tag 70 check) into their own patches? I feel like these
are three distinct fixes and should be treated that way in the git
history.
These are nice improvements! I only have one small, additional comment
below.
>
> Fixes: 237fead61998 ("[PATCH] ecryptfs: fs/Makefile and fs/Kconfig")
> Fixes: 9c79f34f7ee7 ("eCryptfs: Filename Encryption: Tag 70 packets")
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
> ---
> fs/ecryptfs/crypto.c | 2 +-
> fs/ecryptfs/ecryptfs_kernel.h | 3 ++-
> fs/ecryptfs/keystore.c | 32 +++++++++++++++++++++++++++-----
> 3 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index 74b02b55e3f6..e67119b6029c 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -1197,7 +1197,7 @@ static int ecryptfs_read_headers_virt(char *page_virt,
> } else
> set_default_header_data(crypt_stat);
> rc = ecryptfs_parse_packet_set(crypt_stat, (page_virt + offset),
> - ecryptfs_dentry);
> + PAGE_SIZE - offset, ecryptfs_dentry);
> out:
> return rc;
> }
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index f4f56a92bd56..7d2488a10b17 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -580,7 +580,8 @@ int ecryptfs_generate_key_packet_set(char *dest_base,
> size_t *len, size_t max);
> int
> ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
> - unsigned char *src, struct dentry *ecryptfs_dentry);
> + unsigned char *src, size_t src_size,
> + struct dentry *ecryptfs_dentry);
> int ecryptfs_truncate(struct dentry *dentry, loff_t new_length);
> ssize_t
> ecryptfs_getxattr_lower(struct dentry *lower_dentry, struct inode *lower_inode,
> diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> index ebebc9551f1f..888599739274 100644
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -894,6 +894,12 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
> "rc = [%d]\n", __func__, rc);
> goto out;
> }
> + if (s->parsed_tag_70_packet_size < (ECRYPTFS_SIG_SIZE + 2)) {
> + ecryptfs_printk(KERN_WARNING, "Invalid packet size [%zd]\n",
> + s->parsed_tag_70_packet_size);
> + rc = -EINVAL;
> + goto out;
> + }
> s->block_aligned_filename_size = (s->parsed_tag_70_packet_size
> - ECRYPTFS_SIG_SIZE - 1);
> if ((1 + s->packet_size_len + s->parsed_tag_70_packet_size)
> @@ -1537,7 +1543,7 @@ parse_tag_11_packet(unsigned char *data, unsigned char *contents,
> }
> (*packet_size) += length_size;
> (*tag_11_contents_size) = (body_size - 14);
> - if (unlikely((*packet_size) + body_size + 1 > max_packet_size)) {
> + if (unlikely((*packet_size) + body_size > max_packet_size)) {
> printk(KERN_ERR "Packet size exceeds max\n");
> rc = -EINVAL;
> goto out;
> @@ -1704,6 +1710,7 @@ decrypt_passphrase_encrypted_session_key(struct ecryptfs_auth_tok *auth_tok,
> * ecryptfs_parse_packet_set
> * @crypt_stat: The cryptographic context
> * @src: Virtual address of region of memory containing the packets
> + * @src_size: Size of the packet set buffer
> * @ecryptfs_dentry: The eCryptfs dentry associated with the packet set
> *
> * Get crypt_stat to have the file's session key if the requisite key
> @@ -1714,7 +1721,7 @@ decrypt_passphrase_encrypted_session_key(struct ecryptfs_auth_tok *auth_tok,
> * conditions.
> */
> int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
> - unsigned char *src,
> + unsigned char *src, size_t src_size,
> struct dentry *ecryptfs_dentry)
> {
> size_t i = 0;
> @@ -1736,7 +1743,11 @@ int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
> * added the our &auth_tok_list */
> next_packet_is_auth_tok_packet = 1;
> while (next_packet_is_auth_tok_packet) {
> - size_t max_packet_size = ((PAGE_SIZE - 8) - i);
> + size_t max_packet_size;
> +
> + if (i >= src_size)
> + break;
> + max_packet_size = src_size - i;
>
> switch (src[i]) {
> case ECRYPTFS_TAG_3_PACKET_TYPE:
> @@ -1751,12 +1762,16 @@ int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
> goto out_wipe_list;
> }
> i += packet_size;
> + if (i > src_size) {
> + rc = -EIO;
> + goto out_wipe_list;
> + }
> rc = parse_tag_11_packet((unsigned char *)&src[i],
> sig_tmp_space,
> ECRYPTFS_SIG_SIZE,
> &tag_11_contents_size,
> &tag_11_packet_size,
> - max_packet_size);
> + src_size - i);
> if (rc) {
> ecryptfs_printk(KERN_ERR, "No valid "
> "(ecryptfs-specific) literal "
> @@ -1768,6 +1783,10 @@ int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
> goto out_wipe_list;
> }
> i += tag_11_packet_size;
> + if (i > src_size) {
> + rc = -EIO;
> + goto out_wipe_list;
> + }
> if (ECRYPTFS_SIG_SIZE != tag_11_contents_size) {
> ecryptfs_printk(KERN_ERR, "Expected "
> "signature of size [%d]; "
> @@ -1793,6 +1812,10 @@ int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
> goto out_wipe_list;
> }
> i += packet_size;
> + if (i > src_size) {
> + rc = -EIO;
> + goto out_wipe_list;
> + }
> crypt_stat->flags |= ECRYPTFS_ENCRYPTED;
> break;
> case ECRYPTFS_TAG_11_PACKET_TYPE:
> @@ -2480,4 +2503,3 @@ ecryptfs_add_global_auth_tok(struct ecryptfs_mount_crypt_stat *mount_crypt_stat,
> mutex_unlock(&mount_crypt_stat->global_auth_tok_list_mutex);
> return 0;
> }
> -
Please drop this unnecessary line removal.
Tyler
> --
> 2.51.0
>
prev parent reply other threads:[~2026-07-14 4:19 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-27 9:02 Yichong Chen
2026-07-14 4:19 ` Tyler Hicks [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=alW4t1KssnzQbl4Y@elm \
--to=code@tyhicks.com \
--cc=chenyichong@uniontech.com \
--cc=ebiggers@kernel.org \
--cc=ecryptfs@vger.kernel.org \
--cc=kees@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=thorsten.blum@linux.dev \
/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
Powered by JetHome