Re: [PATCH] ecryptfs: validate encoded packet length extent

[Tyler Hicks]

On 2026-07-15 16:37:02, Pengpeng Hou wrote:
> ecryptfs_miscdev_write() accepts a six-byte write as a minimally sized
> message.  It then copies two bytes from the packet-length field at byte
> offset five, although such an input supplies only the first byte.
> 
> Copy only the bytes covered by the current write.  When the first byte
> selects the two-byte encoding, reject the message unless it also supplies
> the second byte before calling ecryptfs_parse_packet_length().
> 
> Fixes: 8bf2debd5f7b ("eCryptfs: introduce device handle for userspace daemon communications")
> Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>

Hello and thank you for the fix! Some comments below...

> ---
>  fs/ecryptfs/miscdev.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ecryptfs/miscdev.c b/fs/ecryptfs/miscdev.c
> index 5a7d08149922..a99e16db8243 100644
> --- a/fs/ecryptfs/miscdev.c
> +++ b/fs/ecryptfs/miscdev.c
> @@ -360,7 +360,7 @@ ecryptfs_miscdev_write(struct file *file, const char __user *buf,
>  	u32 seq;
>  	size_t packet_size, packet_size_length;
>  	char *data;
> -	unsigned char packet_size_peek[ECRYPTFS_MAX_PKT_LEN_SIZE];
> +	unsigned char packet_size_peek[ECRYPTFS_MAX_PKT_LEN_SIZE] = { 0 };
>  	ssize_t rc;
>  
>  	if (count == 0) {
> @@ -376,11 +376,17 @@ ecryptfs_miscdev_write(struct file *file, const char __user *buf,
>  	}
>  
>  	if (copy_from_user(packet_size_peek, &buf[PKT_LEN_OFFSET],
> -			   sizeof(packet_size_peek))) {
> +			   min_t(size_t, count - PKT_LEN_OFFSET,
> +				 sizeof(packet_size_peek)))) {

This is a good change and fixes a potential one-byte over read.

>  		printk(KERN_WARNING "%s: Error while inspecting packet size\n",
>  		       __func__);
>  		return -EFAULT;
>  	}
> +	if (packet_size_peek[0] >= 192 && packet_size_peek[0] < 224 &&
> +	    count - PKT_LEN_OFFSET < ECRYPTFS_MAX_PKT_LEN_SIZE) {
> +		ecryptfs_printk(KERN_WARNING, "Truncated packet length\n");
> +		return -EINVAL;
> +	}

I don't think that this new check is necessary. The call to
ecryptfs_parse_packet_length(), below, will already return a
packet_size_length of 2 if packet_size_peek[0] is >= 192.

The existing check, right after the call to
ecryptfs_parse_packet_length() is error checked, ensures that the value
of count is equal to 1 + 4 + 2 + packet_size. This already protects
against the truncated packet length issue that the new conditional is
checking. Do you agree?

Tyler

>  
>  	rc = ecryptfs_parse_packet_length(packet_size_peek, &packet_size,
>  					  &packet_size_length);
> -- 
> 2.43.0
>