linux-ext4 - Re: [PATCH 3/3] fuse2fs: catch positive errnos coming from libext2fs

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]

Message-ID: <20250612221552.GO6179@frogsfrogsfrogs>
Date: Thu, 12 Jun 2025 15:15:52 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Theodore Ts'o <tytso@....edu>
Cc: linux-ext4@...r.kernel.org
Subject: Re: [PATCH 3/3] fuse2fs: catch positive errnos coming from libext2fs

On Thu, Jun 12, 2025 at 02:13:04PM -0230, Theodore Ts'o wrote:
> On Wed, Jun 11, 2025 at 09:44:17AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@...nel.org>
> > 
> > Something in libext2fs is returning EOVERFLOW (positive) to us.  We need
> > to pass negative errnos back to the fuse driver, so perform this
> > translation.
> 
> This isn't actually the best way to fix things.  The way the com_err
> architecture works is that errcode_t is a 32-bit unsigned integer,
> where the the top 24-bits is a subsystem identifier.  If the subsystem
> identifier is zero, then the low 8 bits is presumed to be an errno
> value.  Otherwise, the subsystem identifier is formed by taking a 4
> character identifier from 62 valid code points A-Z, a-z, 0-9, and _,
> where A is 1, and _ is 63.

I.... had no idea that errcode_t's were actually segmented numbers.  Is
there a way to figure out the subsystem from one of them?  Or will
fuse2fs just have to "know" that !(errcode & 0xFFFFFF00) means "errno"?

> Error table subsystems that are in common use and used by packages
> shipped by most Linux distributions include "krb5" and "kadm" from the
> MIT Kerberos implementation, and "ext2" and "prof" which ship as part
> of e2fsprogs.  The original design came from Multics, and the idea is
> that a library might call other libraries, and having a single unified
> error code namespace can be super-useful.  Top-level callers can check
> to see if an error is non-zero, and if so, call error_message(retval)
> and get back a human-friendly string regardless of which library might
> have originally generated the error.
> 
> CMU has a handy-dandy registry of the various libraries that use the
> common error infrastructure here[1].
> 
> [1] https://www.central.org/frameless/numbers/errors.html
> 
> In the case of the ext2fs library, it doesn't actually call any AFS,
> Kerberos, ASN.1, etc. libraries, so in practice the only valid error
> codes that we should get back are either in the range 0..255 and
> EXT2_ET_BASE..EXT2_ET_BASE+255.  But at least in theory, it's possible
> that in the future, libext2fs might call some other library that might
> return com_err error codes.
> 
> So a better, more idiomatic fix would be something like this below.
> 
>        	       		      	      	- Ted
> 
> P.S.  By the way, I'm not entirely convinced by the is_err vs !is_err
> logic.  I get the idea is that we want to not log certain error cases
> resulting from looking up a non-existing file name, but for example,
> EXT2_ET_NO_MEMORY or any of the EXT2_TDB_* error messages should never
> happen under normal circumstances, so if they do happen, they probably
> should be logged, and so perhaps is_err=1 should be set for those
> errors.  Similarly, I suspect that any MMP errors should probably also
> be logged, but we can handle that as a separate commit.

Hrm -- if MMP fails, that implies that we might not be the owner of
this filesystem, right?  Doesn't that means we should be careful about
not scribbling on the superblock?

> commit 71f046a788adbae163c9398fccf50fff89bb9083
> Author: Theodore Ts'o <tytso@....edu>
> Date:   Thu Jun 12 14:03:44 2025 -0230
> 
>     fuse2fs: correctly handle system errno values in __translate_error()
>     
>     Fixes: 81cbf1ef4f5dab ("misc: add fuse2fs, a FUSE server for e2fsprogs")
>     Reported-by: "Darrick J. Wong" <djwong@...nel.org>
>     Signed-off-by: Theodore Ts'o <tytso@....edu>
> 
> diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
> index bc49edfe..97b1c5b5 100644
> --- a/misc/fuse2fs.c
> +++ b/misc/fuse2fs.c
> @@ -4659,9 +4659,9 @@ static int __translate_error(ext2_filsys fs, ext2_ino_t ino, errcode_t err,
>  	int is_err = 0;
>  
>  	/* Translate ext2 error to unix error code */
> -	if (err < EXT2_ET_BASE)
> -		goto no_translation;
>  	switch (err) {
> +	case 0:
> +		break;
>  	case EXT2_ET_NO_MEMORY:
>  	case EXT2_ET_TDB_ERR_OOM:
>  		ret = -ENOMEM;
> @@ -4755,11 +4755,10 @@ static int __translate_error(ext2_filsys fs, ext2_ino_t ino, errcode_t err,
>  		break;
>  	default:
>  		is_err = 1;
> -		ret = -EIO;
> +		ret = (err < 256) ? -err : -EIO;

Ok I'll rework the patch with this logic, but add a fat comment about
all this.

--D

>  		break;
>  	}
>  
> -no_translation:
>  	if (!is_err)
>  		return ret;
>  
>