Re: [PATCH] xfs_logprint: Fix buffer overflow printing quotaoff

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Mar 16, 2021 at 03:10:44PM +0100, Carlos Maiolino wrote:
> On Tue, Mar 16, 2021 at 08:45:20AM -0500, Eric Sandeen wrote:
> > On 3/16/21 4:04 AM, Carlos Maiolino wrote:
> > > xlog_recover_print_quotaoff() was using a static buffer to aggregate
> > > quota option strings to be printed at the end. The buffer size was
> > > miscalculated and when printing all 3 flags, a buffer overflow occurs
> > > crashing xfs_logprint, like:
> > > 
> > > QOFF: cnt:1 total:1 a:0x560530ff3bb0 len:160
> > > *** buffer overflow detected ***: terminated
> > > Aborted (core dumped)
> > > 
> > > Fix this by removing the static buffer and using printf() directly to
> > > print each flag. 
> > 
> > Yeah, that makes sense. Not sure why it was using a static buffer,
> > unless I was missing something?
> > 
> > > Also add a trailling space before each flag, so they
> > 
> > "trailing space before?" :) I can fix that up on commit.
> 
> Maybe I slipped into my words here... The current printed format, did something
> like this:
> 
> type: USER QUOTAGROUP QUOTAPROJECT QUOTA
> 
> I just added a space before each flag string, to print like this:
> 
> type: USER QUOTA GROUP QUOTA PROJECT QUOTA

Any reason we can't just shorten this to "type: USER GROUP PUOTA" ?

--D

> 
> 
> > >  	if (qoff_f->qf_flags & XFS_UQUOTA_ACCT)
> > > -		strcat(str, "USER QUOTA");
> > > +		printf(" USER QUOTA");
> 			^ here (an in the remaining ones)
> 
> > >  	if (qoff_f->qf_flags & XFS_GQUOTA_ACCT)
> > > -		strcat(str, "GROUP QUOTA");
> > > +		printf(" GROUP QUOTA");
> > >  	if (qoff_f->qf_flags & XFS_PQUOTA_ACCT)
> > > -		strcat(str, "PROJECT QUOTA");
> > > -	printf(_("\tQUOTAOFF: #regs:%d   type:%s\n"),
> > > -	       qoff_f->qf_size, str);
> > > +		printf(" PROJECT QUOTA");
> > 
> > Seems like a clean solution, thanks.
> > 
> > Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> > 
> > > +	printf("\n");
> > >  }
> > >  
> > >  STATIC void
> > > 
> > 
> 
> -- 
> Carlos
>