| Message ID | 1458309822-5550-1-git-send-email-ahferroin7@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Mar 18, 2016 at 10:03:42AM -0400, Austin S. Hemmelgarn wrote: > Currently, open_ctree_fs_info will open whatever path you pass it and try > to interpret it as a BTRFS filesystem. While this is not nessecarily > dangerous (except possibly if done on a character device), it does > result in some rather cryptic and non-sensical error messages when > trying to run certain commands in ways they weren't intended to be run. > Add a check using stat(2) to verify that the path we've been passed is > in fact a regular file or a block device, or a symlink pointing to a > regular file or block device. > > This causes the following commands to provide a helpful error message > when run on a FIFO, directory, character device, or socket: > * btrfs check > * btrfs restore > * btrfs-image > * btrfs-find-root > * btrfs inspect-internal dump-tree > > stat(2) is used instead of lstat(2), as stat(2) follows symlinks just > like open(2) does, which means we check the same inode that open(2) > opens, and thus don't need special handling for symlinks. > > Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com> Applied, thanks. > This has been both build and runtime tested on an x86-64 system with > glibc. It has been build but not runtime tested with uClibc on x86-64 > and ARMv7. It has not been tested on Android or with musl, although it > should work there also. I would not expect any difference among the other arches and libc, it's using a common interface. > There are other tools that have similarly poor error behavior when > called incorrectly (btrfs rescue immediately comes to mind), but they > don't use open_ctree_fs_info, so this doesn't affect them. I may do > followup patches to fix those too if I have the time. Yeah, it would be good to fix all. > Whitelisting is used instead of blacklisting because I feel it provides > a more concise and more easily readable conditional, and because I > think it's a lot less likely that a new file type will be added that > can contain a filesystem image than it is that one will be added which > can't contain a filesystem image (it's more likely that Solaris doors > get ported to Linux than that we get some new file type). Agreed. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-03-18 11:17, David Sterba wrote: > On Fri, Mar 18, 2016 at 10:03:42AM -0400, Austin S. Hemmelgarn wrote: >> This has been both build and runtime tested on an x86-64 system with >> glibc. It has been build but not runtime tested with uClibc on x86-64 >> and ARMv7. It has not been tested on Android or with musl, although it >> should work there also. > > I would not expect any difference among the other arches and libc, it's > using a common interface. The only one I would think might possibly be an issue is Android (bionic has some serious oddities in some places), but I don't think it's likely that they deviated from POSIX semantics here (I would check the documentation, but I don't have it readily available). Mention of the arch was more for the sake of completeness in specifying what I tested than anything else. > >> There are other tools that have similarly poor error behavior when >> called incorrectly (btrfs rescue immediately comes to mind), but they >> don't use open_ctree_fs_info, so this doesn't affect them. I may do >> followup patches to fix those too if I have the time. > > Yeah, it would be good to fix all. I should have time to take a closer look at btrfs rescue early next week, so I may have a patch for that relatively soon. I can't readily think of anything else that might be doing similar things without calling open_ctree_fs_info though, so it may be a bit longer before I actually post a patch (ideally, I'd like to get all the other things that parse unmounted filesystems at the same time). -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-03-18 11:17, David Sterba wrote: > On Fri, Mar 18, 2016 at 10:03:42AM -0400, Austin S. Hemmelgarn wrote: >> There are other tools that have similarly poor error behavior when >> called incorrectly (btrfs rescue immediately comes to mind), but they >> don't use open_ctree_fs_info, so this doesn't affect them. I may do >> followup patches to fix those too if I have the time. > > Yeah, it would be good to fix all. I had a bit of spare time over lunch, and decided to take a cursory look. I hadn't realized that open_ctree uses open_ctree_fs_info, and it looks like almost everything that doesn't call open_ctree_fs_info calls open_ctree, which means this actually added error checking to a lot more than I realized, possibly everything that expects a filesystem image to operate on. I've checked btrfs rescue now, and with this change, it now gives a nice error for something other than a a file or block device. It looks like a follow up patch may not be necessary after all, I'll try and double check the rest of the commands that I haven't checked already over the weekend, and hopefully have something by Monday or Tuesday if there's anything else that needs this check added. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/disk-io.c b/disk-io.c index e520d80..d35153d 100644 --- a/disk-io.c +++ b/disk-io.c @@ -1310,6 +1310,13 @@ struct btrfs_fs_info *open_ctree_fs_info(const char *filename, int fp; struct btrfs_fs_info *info; int oflags = O_CREAT | O_RDWR; + struct stat sb; + + stat(filename, &sb); + if (!(((sb.st_mode & S_IFMT) == S_IFREG) || ((sb.st_mode & S_IFMT) == S_IFBLK))) { + fprintf (stderr, "%s is not a regular file or block device\n", filename); + return NULL; + } if (!(flags & OPEN_CTREE_WRITES)) oflags = O_RDONLY;
Currently, open_ctree_fs_info will open whatever path you pass it and try to interpret it as a BTRFS filesystem. While this is not nessecarily dangerous (except possibly if done on a character device), it does result in some rather cryptic and non-sensical error messages when trying to run certain commands in ways they weren't intended to be run. Add a check using stat(2) to verify that the path we've been passed is in fact a regular file or a block device, or a symlink pointing to a regular file or block device. This causes the following commands to provide a helpful error message when run on a FIFO, directory, character device, or socket: * btrfs check * btrfs restore * btrfs-image * btrfs-find-root * btrfs inspect-internal dump-tree stat(2) is used instead of lstat(2), as stat(2) follows symlinks just like open(2) does, which means we check the same inode that open(2) opens, and thus don't need special handling for symlinks. Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com> --- Changes from v1: * Updated commit message to use the new name for btrfs-debug-tree * Added a bit of clarity to the commit message to explain that stat(2) follows symlinks just like open(2) does, so we don't need special handling for symlinks.. * Added the btrfs-progs developers to the recipient list, because I somehow forgot to add them on the first version. This has been both build and runtime tested on an x86-64 system with glibc. It has been build but not runtime tested with uClibc on x86-64 and ARMv7. It has not been tested on Android or with musl, although it should work there also. There are other tools that have similarly poor error behavior when called incorrectly (btrfs rescue immediately comes to mind), but they don't use open_ctree_fs_info, so this doesn't affect them. I may do followup patches to fix those too if I have the time. open_ctree_fs_info is also used in cmds-filesystem.c, although I'm not at all sure what exactly is going on there, and btrfs filesystem appears from my testing to behave exactly the same with this change, so I don't think this will have any effect on any of the btrfs filesystem commands. Whitelisting is used instead of blacklisting because I feel it provides a more concise and more easily readable conditional, and because I think it's a lot less likely that a new file type will be added that can contain a filesystem image than it is that one will be added which can't contain a filesystem image (it's more likely that Solaris doors get ported to Linux than that we get some new file type). disk-io.c | 7 +++++++ 1 file changed, 7 insertions(+)