xfsprogs/quota: fix NULL pointer dereference in report_f

PATCH In review Jeff Liu 3 participants 9 messages Nov 14, 2013 – Jul 15, 2014

lore

Share & tools

lore

Jeff Liu

author

2013-11-14 09:50 UTC #0 lore raw

PATCH 1/1 xfsprogs/quota: fix NULL pointer dereference in report_f

From: Jie Liu <[email protected]>

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 <[email protected]>

---
 quota/report.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

quota/report.c +1 -1

index 70894a2..8e3316e 100644
@@ -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) {

Signature · 6 lines

1.8.3.2

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2013-11-14 14:09 UTC #1 lore raw

Quoted 9 lines · Jeff Liu

From: Jie Liu <[email protected]>
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)

Quoted 8 lines

(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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Jeff Liu

author

2013-11-15 01:42 UTC #2 lore raw

Quoted 13 lines · Christoph Hellwig

Quoted 12 lines · Jeff Liu
From: Jie Liu <[email protected]>
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

Quoted 8 lines

Quoted 5 lines
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 <[email protected]>
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.

Quoted 14 lines

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)

Quoted 8 lines

{
	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.

Quoted 2 lines

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2013-11-15 14:09 UTC #3 lore raw

Quoted 3 lines · Jeff Liu

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Jeff Liu

author

2013-11-15 15:37 UTC #4 lore raw

Quoted 8 lines · Christoph Hellwig

Quoted 3 lines · Jeff Liu
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Jeff Liu

author

2013-11-16 02:54 UTC #5 lore raw

Quoted 6 lines · Jeff Liu

Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Jeff Liu
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():

Quoted 6 lines

Quoted 3 lines
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

Reviewed-by on v1 1/1

2014-07-12 01:39 UTC #6 lore raw

In reply to Jeff Liu

Quoted 12 lines · Jeff Liu

From: Jie Liu <[email protected]>
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

Quoted 7 lines

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 <[email protected]>

Reviewed-by Eric Sandeen <[email protected]>

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

Quoted 10 lines

---
 quota/report.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

quota/report.c +0 -0

index 70894a2..8e3316e 100644

quota/report.c report_f( · line 630

Quoted 8 lines

quota/report.c +1 -1

@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

Reviewed-by

2014-07-15 10:14 UTC #7 lore raw

Quoted 8 lines · Eric Sandeen

Quoted 1 line
Signed-off-by Jie Liu <[email protected]>
Reviewed-by Eric Sandeen <[email protected]>
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 <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2014-07-15 12:54 UTC #8 lore raw

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Eric Sandeen
Quoted 1 line
Signed-off-by Jie Liu <[email protected]>
Reviewed-by Eric Sandeen <[email protected]>

Quoted 7 lines

Quoted 4 lines
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

Quoted 5 lines

either way, consider this:
Reviewed-by Christoph Hellwig <[email protected]>

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs