[PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

[PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

[Jeff Liu]

From: Jie Liu <jeff.liu@oracle.com>

Run xfs_quota report against an invalid XFS path without desired quota
limitation is enabled will hit SEGSEGV as fs_path is uninitialized, e.g.

# xfs_quota -xc 'report -up' /invalid_path
xfs_quota: cannot setup path for mount /invalid_path: No such file or directory
Segmentation fault (core dumped)

(gdb) r -xc 'report -up' /invalid_path
xfs_quota: cannot setup path for mount /invalid_path: No such file or directory

Program received signal SIGSEGV, Segmentation fault.
0x0000000000408b4d in report_f (argc=2, argv=0x105ea70) at report.c:627
627			else if (fs_path->fs_flags & FS_MOUNT_POINT)

This patch fix report_f() to only do report if the fs_path is initialized.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
 quota/report.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/quota/report.c b/quota/report.c
index 70894a2..8e3316e 100644
--- a/quota/report.c
+++ b/quota/report.c
@@ -624,7 +624,7 @@ report_f(
 		if (flags & ALL_MOUNTS_FLAG)
 			report_any_type(fp, form, type, NULL,
 					lower, upper, flags);
-		else if (fs_path->fs_flags & FS_MOUNT_POINT)
+		else if (fs_path && (fs_path->fs_flags & FS_MOUNT_POINT))
 			report_any_type(fp, form, type, fs_path->fs_dir,
 					lower, upper, flags);
 	} else while (argc > optind) {
-- 
1.8.3.2

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

[Christoph Hellwig]

On Thu, Nov 14, 2013 at 05:49:57PM +0800, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> Run xfs_quota report against an invalid XFS path without desired quota
> limitation is enabled will hit SEGSEGV as fs_path is uninitialized, e.g.
> 
> # xfs_quota -xc 'report -up' /invalid_path
> xfs_quota: cannot setup path for mount /invalid_path: No such file or directory
> Segmentation fault (core dumped)
> 
> (gdb) r -xc 'report -up' /invalid_path
> xfs_quota: cannot setup path for mount /invalid_path: No such file or directory
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000000408b4d in report_f (argc=2, argv=0x105ea70) at report.c:627
> 627			else if (fs_path->fs_flags & FS_MOUNT_POINT)
> 
> This patch fix report_f() to only do report if the fs_path is initialized.

quot_f in quot.c and various functions in state.c seem to have the same
issue, and need fixing as well.

I'd love to avoid having to expose fs_path as a global and just have a
few safe accessors:

char *global_fsname(void)
{
	return fs_path ? fs_path->fs_name : NULL;
}

char *global_dir(void)
{
	return fs_path ? fs_path->fs_dir : NULL;
}

char *global_mountpoint(void)
{
	if (fs_path && (fs_path->fs_flags & FS_MOUNT_POINT))
		return fs_path->fs_dir;
	return NULL;
}

Together with moving a bit of code from quota/init.c to libxcmd/path.c
that should allow mkaing fs_path and fs_table static there.


I'd also really love to see a regression test for xfstests.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

[Jeff Liu]

On 11/14 2013 22:09 PM, Christoph Hellwig wrote:
> On Thu, Nov 14, 2013 at 05:49:57PM +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@oracle.com>
>>
>> Run xfs_quota report against an invalid XFS path without desired quota
>> limitation is enabled will hit SEGSEGV as fs_path is uninitialized, e.g.
>>
>> # xfs_quota -xc 'report -up' /invalid_path
>> xfs_quota: cannot setup path for mount /invalid_path: No such file or directory
>> Segmentation fault (core dumped)
>>
>> (gdb) r -xc 'report -up' /invalid_path
>> xfs_quota: cannot setup path for mount /invalid_path: No such file or directory
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x0000000000408b4d in report_f (argc=2, argv=0x105ea70) at report.c:627
>> 627			else if (fs_path->fs_flags & FS_MOUNT_POINT)
>>
>> This patch fix report_f() to only do report if the fs_path is initialized.
> 
> quot_f in quot.c and various functions in state.c seem to have the same
> issue, and need fixing as well.
As per a rough tryout while fixing this problem, it seems that most of those
functions are just not be invoked since other precheck-ups were failed before
calling them.  We have another old fix for stat_f():

commit 04418c5901b846cf904d929210cb71dfba44e9a7
Author: Donald Douwsma <donaldd@sgi.com>
Date:   Mon Jul 23 04:14:27 2007 +0000

    Fix null pointer dereference in state_f.
    Merge of master-melb:xfs-cmds:29194a by kenmcd.

> I'd love to avoid having to expose fs_path as a global and just have a
> few safe accessors:
> 
> char *global_fsname(void)
> {
> 	return fs_path ? fs_path->fs_name : NULL;
> }
> 
> char *global_dir(void)
> {
> 	return fs_path ? fs_path->fs_dir : NULL;
> }
> 
> char *global_mountpoint(void)
> {
> 	if (fs_path && (fs_path->fs_flags & FS_MOUNT_POINT))
> 		return fs_path->fs_dir;
> 	return NULL;
> }
> 
> Together with moving a bit of code from quota/init.c to libxcmd/path.c
> that should allow mkaing fs_path and fs_table static there.
Sounds reasonable to me, thanks for your suggestion.
> 
> I'd also really love to see a regression test for xfstests.
I'll take care of it, let me double check all those sub-commands at first.

Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

[Christoph Hellwig]

On Fri, Nov 15, 2013 at 09:42:03AM +0800, Jeff Liu wrote:
> As per a rough tryout while fixing this problem, it seems that most of those
> functions are just not be invoked since other precheck-ups were failed before
> calling them.  We have another old fix for stat_f():

Most or all?  If this is the only case I'm fine with putting your
patch in a quick fix, even if I'd prefer the version I suggested for
the long run.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

[Jeff Liu]

On 11/15 2013 22:09, Christoph Hellwig wrote:
> On Fri, Nov 15, 2013 at 09:42:03AM +0800, Jeff Liu wrote:
>> As per a rough tryout while fixing this problem, it seems that most of those
>> functions are just not be invoked since other precheck-ups were failed before
>> calling them.  We have another old fix for stat_f():
> 
> Most or all?  If this is the only case I'm fine with putting your
> patch in a quick fix, even if I'd prefer the version I suggested for
> the long run.
Most.  I'm in the middle of this check because I found another critical
bug in kernel quota management which can cause xfs_quota hang up, it can
100% reproducible but still under investigation.

Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

[Jeff Liu]

On 11/15 2013 23:36 PM, Jeff Liu wrote:
> On 11/15 2013 22:09, Christoph Hellwig wrote:
>> On Fri, Nov 15, 2013 at 09:42:03AM +0800, Jeff Liu wrote:
>>> As per a rough tryout while fixing this problem, it seems that most of those
>>> functions are just not be invoked since other precheck-ups were failed before
>>> calling them.  We have another old fix for stat_f():
>>
>> Most or all?  If this is the only case I'm fine with putting your
>> patch in a quick fix, even if I'd prefer the version I suggested for
>> the long run.
> Most.  I'm in the middle of this check because I found another critical
> bug in kernel quota management which can cause xfs_quota hang up, it can
> 100% reproducible but still under investigation.
Sorry for the delay!  I can not find more problems.

AFAICS, there are 6 administrator commands dereference fs_path for further
operation, i.e, free, path, limit, quot, state, report.
Except report, state has already been fixed with fs_path precheck-up, the
others are safe as additional precheck-ups failed.

Thanks,
-Jeff


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

[Eric Sandeen]

On 11/14/13, 3:49 AM, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> Run xfs_quota report against an invalid XFS path without desired quota
> limitation is enabled will hit SEGSEGV as fs_path is uninitialized, e.g.
> 
> # xfs_quota -xc 'report -up' /invalid_path
> xfs_quota: cannot setup path for mount /invalid_path: No such file or directory
> Segmentation fault (core dumped)
> 
> (gdb) r -xc 'report -up' /invalid_path
> xfs_quota: cannot setup path for mount /invalid_path: No such file or directory
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000000408b4d in report_f (argc=2, argv=0x105ea70) at report.c:627
> 627			else if (fs_path->fs_flags & FS_MOUNT_POINT)
> 
> This patch fix report_f() to only do report if the fs_path is initialized.
> 
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

I know hch had grand plans to make this all nicer, but based on Jeff's
followup emails, it looks like this is the only spot needed to simply
resolve the problems w/ nonexistent paths in quota, and is a small,
contained change we can easily make before the next point release.

Thanks,
-Eric

> ---
>  quota/report.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/quota/report.c b/quota/report.c
> index 70894a2..8e3316e 100644
> --- a/quota/report.c
> +++ b/quota/report.c
> @@ -624,7 +624,7 @@ report_f(
>  		if (flags & ALL_MOUNTS_FLAG)
>  			report_any_type(fp, form, type, NULL,
>  					lower, upper, flags);
> -		else if (fs_path->fs_flags & FS_MOUNT_POINT)
> +		else if (fs_path && (fs_path->fs_flags & FS_MOUNT_POINT))
>  			report_any_type(fp, form, type, fs_path->fs_dir,
>  					lower, upper, flags);
>  	} else while (argc > optind) {
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

[Christoph Hellwig]

On Fri, Jul 11, 2014 at 08:39:45PM -0500, Eric Sandeen wrote:
> > Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> I know hch had grand plans to make this all nicer, but based on Jeff's
> followup emails, it looks like this is the only spot needed to simply
> resolve the problems w/ nonexistent paths in quota, and is a small,
> contained change we can easily make before the next point release.

Didn't I say I'm fine with putting this in earlier even if I'd prefer
a grand solution?

either way, consider this:


Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

[Eric Sandeen]

On 7/15/14, 5:14 AM, Christoph Hellwig wrote:
> On Fri, Jul 11, 2014 at 08:39:45PM -0500, Eric Sandeen wrote:
>>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>>
>> I know hch had grand plans to make this all nicer, but based on Jeff's
>> followup emails, it looks like this is the only spot needed to simply
>> resolve the problems w/ nonexistent paths in quota, and is a small,
>> contained change we can easily make before the next point release.
> 
> Didn't I say I'm fine with putting this in earlier even if I'd prefer
> a grand solution?

Yes, you did, but it never got a formal reviewed-by or got merged, so I bumped it :)


FWIW I did look at the grand solution, and there was .. something that
didn't make sense to me, so I backed off.  Oh, yeah, I wasn't quite
sure what to do with:

add_args_command(init_args_command);
and
init_args_command();

in quota/init.c  

-Eric



> either way, consider this:
> 
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs