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] [day] [month] [year] [list]
Message-ID: <ZwrzxggtS96n72Bm@dread.disaster.area>
Date: Sun, 13 Oct 2024 09:10:14 +1100
From: Dave Chinner <david@...morbit.com>
To: Chi Zhiling <chizhiling@....com>
Cc: "Darrick J. Wong" <djwong@...nel.org>, cem@...nel.org,
	linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	chizhiling <chizhiling@...inos.cn>
Subject: Re: [PATCH] xfs_logprint: Fix super block buffer interpretation issue

On Fri, Oct 11, 2024 at 11:54:08AM +0800, Chi Zhiling wrote:
> 
> On 2024/10/11 11:24, Darrick J. Wong wrote:
> > On Fri, Oct 11, 2024 at 11:08:10AM +0800, Chi Zhiling wrote:
> > > From: chizhiling <chizhiling@...inos.cn>
> > > 
> > > When using xfs_logprint to interpret the buffer of the super block, the
> > > icount will always be 6360863066640355328 (0x5846534200001000). This is
> > > because the offset of icount is incorrect, causing xfs_logprint to
> > > misinterpret the MAGIC number as icount.
> > > This patch fixes the offset value of the SB counters in xfs_logprint.
> > > 
> > > Before this patch:
> > > icount: 6360863066640355328  ifree: 5242880  fdblks: 0  frext: 0
> > > 
> > > After this patch:
> > > icount: 10240  ifree: 4906  fdblks: 37  frext: 0
> > > 
> > > Signed-off-by: chizhiling <chizhiling@...inos.cn>
> > > ---
> > >   logprint/log_misc.c | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/logprint/log_misc.c b/logprint/log_misc.c
> > > index 8e86ac34..21da5b8b 100644
> > > --- a/logprint/log_misc.c
> > > +++ b/logprint/log_misc.c
> > > @@ -288,13 +288,13 @@ xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops)
> > >   			/*
> > >   			 * memmove because *ptr may not be 8-byte aligned
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This is important. I'll come back to it.

> > >   			 */
> > > -			memmove(&a, *ptr, sizeof(__be64));
> > > -			memmove(&b, *ptr+8, sizeof(__be64));
> > How did this ever work??  This even looks wrong in "Release_1.0.0".
> > 
> Yes, I was surprised when I find this issue

I"ve never cared about these values when doing diagnosis because
lazy-count means they aren't guaranteed to be correct except at
unmount. At which point, the correct values are generally found
in the superblock. IOWs, the values are largely meaningless whether
they are correct or not, so nobody has really cared enough about
this to bother fixing it...

> > > +			memmove(&a, *ptr + offsetof(struct xfs_dsb, sb_icount), sizeof(__be64));
> > > +			memmove(&b, *ptr + offsetof(struct xfs_dsb, sb_ifree), sizeof(__be64));
> > Why not do:
> > 
> > 			struct xfs_dsb *dsb = *ptr;
> > 
> > 			memcpy(&a, &dsb->sb_icount, sizeof(a));
> > 
> > or better yet, skip the indirection and do
> > 
> > 			printf(_("icount: %llu  ifree: %llu  "),
> > 					(unsigned long long)be64_to_cpu(dsb->sb_icount),
> > 					(unsigned long long)be64_to_cpu(dsb->sb_ifree));
> > 
> > Hm?
> 
> Yes, of course we can do it this way, I just want the fix patch to look
> smaller :)

This won't work on platforms where we can't do 4-byte aligned 64 bit
accesses (various risc archs), hence the comment above about using
memmove.

It should work if we use get_unaligned_be64() rather than
be64_to_cpu() - I would suggest that all such structure decoding in
logprint would need to do the same thing...

-Dave.

-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ