diff mbox series

xfs_logprint: Fix buffer overflow printing quotaoff

Message ID 20210316090400.35180-1-cmaiolino@redhat.com (mailing list archive)
State Superseded
Headers show
Series xfs_logprint: Fix buffer overflow printing quotaoff | expand

Commit Message

Carlos Maiolino March 16, 2021, 9:04 a.m. UTC
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. Also add a trailling space before each flag, so they
are a bit more readable on the output.

Reported-by: Eric Sandeen <sandeen@sandeen.net>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 logprint/log_print_all.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Eric Sandeen March 16, 2021, 1:45 p.m. UTC | #1
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.

> are a bit more readable on the output.
> 
> Reported-by: Eric Sandeen <sandeen@sandeen.net>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  logprint/log_print_all.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/logprint/log_print_all.c b/logprint/log_print_all.c
> index 20f2a445..03a32331 100644
> --- a/logprint/log_print_all.c
> +++ b/logprint/log_print_all.c
> @@ -186,18 +186,18 @@ xlog_recover_print_quotaoff(
>  	struct xlog_recover_item *item)
>  {
>  	xfs_qoff_logformat_t	*qoff_f;
> -	char			str[32] = { 0 };
>  
>  	qoff_f = (xfs_qoff_logformat_t *)item->ri_buf[0].i_addr;
> +
>  	ASSERT(qoff_f);
> +	printf(_("\tQUOTAOFF: #regs:%d   type:"), qoff_f->qf_size);

Ok, we printed this unconditionally before, anyway.

>  	if (qoff_f->qf_flags & XFS_UQUOTA_ACCT)
> -		strcat(str, "USER QUOTA");
> +		printf(" USER QUOTA");
>  	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 Maiolino March 16, 2021, 2:10 p.m. UTC | #2
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


> >  	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
> > 
>
Darrick J. Wong March 16, 2021, 3:36 p.m. UTC | #3
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
>
Eric Sandeen March 16, 2021, 4:11 p.m. UTC | #4
On 3/16/21 10:36 AM, Darrick J. Wong wrote:
> 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" ?

oh yeah, that's a good idea, but: "USER GROUP PROJECT"

-Eric
Carlos Maiolino March 23, 2021, 9:08 a.m. UTC | #5
On Tue, Mar 16, 2021 at 11:11:08AM -0500, Eric Sandeen wrote:
> On 3/16/21 10:36 AM, Darrick J. Wong wrote:
> > 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" ?
> 
> oh yeah, that's a good idea, but: "USER GROUP PROJECT"

Sorry late reply, I don't see why not too, I just followed the current
convention. I'll submit a V2

> 
> -Eric
>
diff mbox series

Patch

diff --git a/logprint/log_print_all.c b/logprint/log_print_all.c
index 20f2a445..03a32331 100644
--- a/logprint/log_print_all.c
+++ b/logprint/log_print_all.c
@@ -186,18 +186,18 @@  xlog_recover_print_quotaoff(
 	struct xlog_recover_item *item)
 {
 	xfs_qoff_logformat_t	*qoff_f;
-	char			str[32] = { 0 };
 
 	qoff_f = (xfs_qoff_logformat_t *)item->ri_buf[0].i_addr;
+
 	ASSERT(qoff_f);
+	printf(_("\tQUOTAOFF: #regs:%d   type:"), qoff_f->qf_size);
 	if (qoff_f->qf_flags & XFS_UQUOTA_ACCT)
-		strcat(str, "USER QUOTA");
+		printf(" USER QUOTA");
 	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");
+	printf("\n");
 }
 
 STATIC void