| Message ID | 20210316090400.35180-1-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs_logprint: Fix buffer overflow printing quotaoff | expand |
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 >
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 > > >
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 >
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
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 --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
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(-)