* [PATCH] xfs_logprint: Fix buffer overflow printing quotaoff
@ 2021-03-16 9:04 Carlos Maiolino
2021-03-16 13:45 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Carlos Maiolino @ 2021-03-16 9:04 UTC (permalink / raw)
To: linux-xfs
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(-)
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
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_logprint: Fix buffer overflow printing quotaoff
2021-03-16 9:04 [PATCH] xfs_logprint: Fix buffer overflow printing quotaoff Carlos Maiolino
@ 2021-03-16 13:45 ` Eric Sandeen
2021-03-16 14:10 ` Carlos Maiolino
0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2021-03-16 13:45 UTC (permalink / raw)
To: Carlos Maiolino, linux-xfs
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_logprint: Fix buffer overflow printing quotaoff
2021-03-16 13:45 ` Eric Sandeen
@ 2021-03-16 14:10 ` Carlos Maiolino
2021-03-16 15:36 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Carlos Maiolino @ 2021-03-16 14:10 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
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
> >
>
--
Carlos
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_logprint: Fix buffer overflow printing quotaoff
2021-03-16 14:10 ` Carlos Maiolino
@ 2021-03-16 15:36 ` Darrick J. Wong
2021-03-16 16:11 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2021-03-16 15:36 UTC (permalink / raw)
To: Eric Sandeen, linux-xfs
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_logprint: Fix buffer overflow printing quotaoff
2021-03-16 15:36 ` Darrick J. Wong
@ 2021-03-16 16:11 ` Eric Sandeen
2021-03-23 9:08 ` Carlos Maiolino
0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2021-03-16 16:11 UTC (permalink / raw)
To: Darrick J. Wong, linux-xfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_logprint: Fix buffer overflow printing quotaoff
2021-03-16 16:11 ` Eric Sandeen
@ 2021-03-23 9:08 ` Carlos Maiolino
0 siblings, 0 replies; 6+ messages in thread
From: Carlos Maiolino @ 2021-03-23 9:08 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Darrick J. Wong, linux-xfs
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
>
--
Carlos
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-23 9:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-16 9:04 [PATCH] xfs_logprint: Fix buffer overflow printing quotaoff Carlos Maiolino
2021-03-16 13:45 ` Eric Sandeen
2021-03-16 14:10 ` Carlos Maiolino
2021-03-16 15:36 ` Darrick J. Wong
2021-03-16 16:11 ` Eric Sandeen
2021-03-23 9:08 ` Carlos Maiolino
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox