mkfs.ubifs problem when output file really isn't in the UBIFS root directory

mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Marcus Prebble]

Hi,

We are working on a project using UBI/UBIFS and I have run into a
problem with mkfs.ubifs which I am hoping someone can advise on.
Apologies if I have already missed the answer to this in the archives
somewhere.

A requirement of the project is to take an image of the root filesystem
(/) which is implemented as a static UBI read only volume. Obviously its
not possible to unmount /, but it seems to be possible to re-mount the
UBI volume, read only, onto another location e.g. /tmp/mnt/new_rootfs.

The problem arises when running mkfs.ubifs is that it fails saying that
the output file cannot be in the UBIFS root directory.
This sounds like a reasonable constraint if the output file really was
in the root, but this is not the case for us seeing root is mounted
somewhere belowtmp. To give a concrete example of the command we are
running:

mkfs.ubifs --output=/tmp/rootfs.img  --leb-size=129024
--root=/tmp/rootfs_mnt --min-io-size=2048 --max-leb-cnt=147 

Where /dev/ubi0_11 is mounted read-only on both / and /tmp/rootfs_mnt/.

Having a look at the source for mkfs.ubifs it seems that the function
in_path(..) is the problem - it works up the directory tree using ../
until it either reaches the top_dir (/) or the inode number/dev match in
which case it fails. It doesn't seem that this section of code has
changed in the latest version of mtd-utils (we are running 1.4.3-2) and
I wonder if this is a bug in in_path(..) or a genuine safeguard against
screwing something up? I suspect the former for when I disabled this
check in mkfs.ubifs.c the resulting image was completely OK.

Kind regards,

Marcus Prebble
Axis Communications AB

Re: mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 2124 bytes --]

On Fri, 2012-09-21 at 17:41 +0200, Marcus Prebble wrote:
> Hi,
> 
> We are working on a project using UBI/UBIFS and I have run into a
> problem with mkfs.ubifs which I am hoping someone can advise on.
> Apologies if I have already missed the answer to this in the archives
> somewhere.
> 
> A requirement of the project is to take an image of the root filesystem
> (/) which is implemented as a static UBI read only volume. Obviously its
> not possible to unmount /, but it seems to be possible to re-mount the
> UBI volume, read only, onto another location e.g. /tmp/mnt/new_rootfs.
> 
> The problem arises when running mkfs.ubifs is that it fails saying that
> the output file cannot be in the UBIFS root directory.
> This sounds like a reasonable constraint if the output file really was
> in the root, but this is not the case for us seeing root is mounted
> somewhere belowtmp. To give a concrete example of the command we are
> running:
> 
> mkfs.ubifs --output=/tmp/rootfs.img  --leb-size=129024
> --root=/tmp/rootfs_mnt --min-io-size=2048 --max-leb-cnt=147 
> 
> Where /dev/ubi0_11 is mounted read-only on both / and /tmp/rootfs_mnt/.
> 
> Having a look at the source for mkfs.ubifs it seems that the function
> in_path(..) is the problem - it works up the directory tree using ../
> until it either reaches the top_dir (/) or the inode number/dev match in
> which case it fails. It doesn't seem that this section of code has
> changed in the latest version of mtd-utils (we are running 1.4.3-2) and
> I wonder if this is a bug in in_path(..) or a genuine safeguard against
> screwing something up? I suspect the former for when I disabled this
> check in mkfs.ubifs.c the resulting image was completely OK.

Well, obviously this is a safeguard check. If your source FS is
in /a/path/, and the output image file is /a/path/image, then we will
loop forever reading /a/path/image and writing the output
to /a/path/image.

In your case 'tmp' is a tmpfs, right? This is obviously a bug then. You
should probably analyze it and send a fix.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Marcus Prebble]

Hi Artem,

Thanks for your reply - please see below.


On Thu, 2012-10-04 at 18:16 +0200, Artem Bityutskiy wrote:
> On Fri, 2012-09-21 at 17:41 +0200, Marcus Prebble wrote:
> > Hi,
> > 
> > We are working on a project using UBI/UBIFS and I have run into a
> > problem with mkfs.ubifs which I am hoping someone can advise on.
> > Apologies if I have already missed the answer to this in the archives
> > somewhere.
> > 
> > A requirement of the project is to take an image of the root filesystem
> > (/) which is implemented as a static UBI read only volume. Obviously its
> > not possible to unmount /, but it seems to be possible to re-mount the
> > UBI volume, read only, onto another location e.g. /tmp/mnt/new_rootfs.
> > 
> > The problem arises when running mkfs.ubifs is that it fails saying that
> > the output file cannot be in the UBIFS root directory.
> > This sounds like a reasonable constraint if the output file really was
> > in the root, but this is not the case for us seeing root is mounted
> > somewhere belowtmp. To give a concrete example of the command we are
> > running:
> > 
> > mkfs.ubifs --output=/tmp/rootfs.img  --leb-size=129024
> > --root=/tmp/rootfs_mnt --min-io-size=2048 --max-leb-cnt=147 
> > 
> > Where /dev/ubi0_11 is mounted read-only on both / and /tmp/rootfs_mnt/.
> > 
> > Having a look at the source for mkfs.ubifs it seems that the function
> > in_path(..) is the problem - it works up the directory tree using ../
> > until it either reaches the top_dir (/) or the inode number/dev match in
> > which case it fails. It doesn't seem that this section of code has
> > changed in the latest version of mtd-utils (we are running 1.4.3-2) and
> > I wonder if this is a bug in in_path(..) or a genuine safeguard against
> > screwing something up? I suspect the former for when I disabled this
> > check in mkfs.ubifs.c the resulting image was completely OK.
> 
> Well, obviously this is a safeguard check. If your source FS is
> in /a/path/, and the output image file is /a/path/image, then we will
> loop forever reading /a/path/image and writing the output
> to /a/path/image.

I am well aware of the need for a safe-guard check, but I think you
mis-understood my question.

We have remounted a static UBI volume which happens to be the rootfs (/)
onto another location, /tmp/newrootfs
We are trying to make an image of /tmp/newrootfs with mkfs.ubifs and
where the output file is /tmp/newrootfs.img.

In other words, we have /foo/bar/ and /foo/out.img. 
out.img is not in the same directory as bar, so it should be legal to
make an image of /foo/bar/

The problem is that in_path() compares inode numbers, and seeing the
inode numbers are apparently the same for / and /tmp/newrootfs
mkfs.ubifs (wrongly) says the output is in the root directory.

My original question was is there any technical reason why in_path()
compares inode numbers? I.e. is there a risk if you make an image of a
re-mounted RO filesystem of the resulting image being corrupt (or
something)

We have a workaround at moment --disable-root-check but its clearly not
an optimal solution. Another user is also having problems with in_path()
so this function needs looking at. Once we know that this is not the
intended behaviour of in_path() then we are happy to look for a
solution.

Kind regards,

/Marcus Prebble

Re: mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

Yeah, I am probably confused.

On Mon, 2012-10-08 at 17:04 +0200, Marcus Prebble wrote:
> We have remounted a static UBI volume which happens to be the rootfs (/)
> onto another location, /tmp/newrootfs

So, you have your UBI volume, it contains your root file-sytem, right?
Then you "remount your" root to /tmp/newrootfs? If yes, I would like to
understand how exactly you do this? Give some more details.

> We are trying to make an image of /tmp/newrootfs with mkfs.ubifs and
> where the output file is /tmp/newrootfs.img.


If the inode number of / is the same as /tmp/newroofs, and the device
number is the same, this is the same inode, no?

This means that /tmp/newrootfs.img will also be visible
in /tmp/newrootfs/tmp/newrootfs.img, no?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Marcus Prebble]

On Wed, 2012-10-10 at 08:47 +0200, Artem Bityutskiy wrote:
> Yeah, I am probably confused.

Or, I have explained it in a not-so-clear way ;)

> 
> On Mon, 2012-10-08 at 17:04 +0200, Marcus Prebble wrote:
> > We have remounted a static UBI volume which happens to be the rootfs (/)
> > onto another location, /tmp/newrootfs
> 
> So, you have your UBI volume, it contains your root file-sytem, right?

Exactly.

> Then you "remount your" root to /tmp/newrootfs? If yes, I would like to
> understand how exactly you do this? Give some more details.

The mount command is: 

mount -t ubifs -o ro ubi0:rootfs /tmp/newrootfs

which results in the following mount table:

[root@axis-00408c93d266 /tmp]4517# mount
ubi0:rootfs-11 on / type ubifs (ro,relatime)
proc on /proc type proc (rw,relatime)
sysfs on /sys type sysfs (rw,relatime)
udev on /dev type tmpfs (rw,relatime)
/dev/ubi0_23 on /mnt/flash type ubifs (rw,relatime)
/dev/part/persistent on /lib/persistent type ubifs (rw,relatime)
/dev/part/iv on /usr/local type ubifs (rw,relatime)
tmpfs on /var type tmpfs (rw,relatime)
devpts on /dev/pts type devpts (rw,relatime,mode=600)
tmpfs on /dev/shm type tmpfs (rw,relatime)
tmpfs on /var/cache/recorder type tmpfs
(rw,relatime,size=111320k,nr_inodes=0)
ubi0:rootfs-11 on /var/tmp/newrootfs type ubifs (ro,relatime)

> 
> > We are trying to make an image of /tmp/newrootfs with mkfs.ubifs and
> > where the output file is /tmp/newrootfs.img.

> If the inode number of / is the same as /tmp/newroofs, and the device
> number is the same, this is the same inode, no?

I wrote a small program to test this seeing we don't have stat on target

[root@axis-00408c93d266 /tmp]4530# ./a.out / 
Stat() info for '/':
st_mode=0x41ed
st_ino=1
st_dev=10
st_rdev=0
st_size=0

[root@axis-00408c93d266 /tmp]4530# ./a.out /tmp/newrootfs/
Stat() info for '/tmp/newrootfs/':
st_mode=0x41ed
st_ino=1
st_dev=10
st_rdev=0
st_size=0

So according to stat() it is the same inode.

> 
> This means that /tmp/newrootfs.img will also be visible
> in /tmp/newrootfs/tmp/newrootfs.img, no?

No, as /tmp is a tmpfs mounted on  /var/tmp

[root@axis-00408c93d266 /]4530# touch /tmp/foo.txt   
[root@axis-00408c93d266 /]4530# ls /tmp/newrootfs/tmp/       
ls: /tmp/newrootfs/tmp/: No such file or directory
[root@axis-00408c93d266 /]4530# 

.. and is thus not reachable from /tmp/newrootfs/ 

[root@axis-00408c93d266 /]4530# ls /tmp/newrootfs/var/
[root@axis-00408c93d266 /]4530# 

Apologies for the long-winded explanation - but the output file really
isn't in the the root directory, even though apparently the inodes are
the same. 

Thanks and kind regards,

/Marcus Prebble

Re: mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On Wed, 2012-10-10 at 11:04 +0200, Marcus Prebble wrote:
> Apologies for the long-winded explanation - but the output file really
> isn't in the the root directory, even though apparently the inodes are
> the same. 

Well, ok, this is a bug. But I think the solution for this should _not_
be a separate "--band-aid" type of option. We should kill 'in_path()'
and implement a 'is_contained(file, dir)' function, which would return
true if 'dir' contains 'file' and false otherwise.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Marcus Prebble]

On Wed, 2012-10-10 at 11:30 +0200, Artem Bityutskiy wrote:
> On Wed, 2012-10-10 at 11:04 +0200, Marcus Prebble wrote:
> > Apologies for the long-winded explanation - but the output file really
> > isn't in the the root directory, even though apparently the inodes are
> > the same. 
> 
> Well, ok, this is a bug. But I think the solution for this should _not_
> be a separate "--band-aid" type of option.

Agree entirely. --band-aid was just used as a proof-of-concept.

>  We should kill 'in_path()'
> and implement a 'is_contained(file, dir)' function, which would return
> true if 'dir' contains 'file' and false otherwise.

I will implement and come back with a patch when I have time.

Thanks again for your input,

/Marcus


> 

Re: mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Artem Bityutskiy]

[-- Attachment #1.1: Type: text/plain, Size: 5181 bytes --]

On Wed, 2012-10-10 at 12:34 +0200, Marcus Prebble wrote:
> I will implement and come back with a patch when I have time.

I actually went ahead and cooked something. Could you please review and
give it a test? Inlined below and also attached. Thanks!


From 7c1b939f2cf0141dfb1969d51b4157a75f55ddac Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Wed, 10 Oct 2012 14:23:18 +0300
Subject: [PATCH] mkfs.ubifs: rewrite path checking

We use the 'in_path()' function to check whether the output image is
withing the mkfs.ubifs root directory or not. However, this function
is not correct and it fails for the following situation, as
Marcus Prebble <marcus.prebble@axis.com> reports:

1. We have our root file-system mounted at / and want to build an image
   out of it.
2. We have tmpfs mounted at /tmp
3. We mount the root file-system under /tmp/newroot
4. We run mkfs.ubifs with -r /tmp/newroot -o /tmp/image

And this fails. It fails because 'in_path()' misses this use-case.

This patch re-implements the check completely. Now we use 'realpath()'
to find canonical paths and just check that the output file is not
under the root mkfs.ubifs directory.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 mkfs.ubifs/mkfs.ubifs.c | 116 +++++++++++++++---------------------------------
 1 file changed, 37 insertions(+), 79 deletions(-)

diff --git a/mkfs.ubifs/mkfs.ubifs.c b/mkfs.ubifs/mkfs.ubifs.c
index 149806b..885c202 100644
--- a/mkfs.ubifs/mkfs.ubifs.c
+++ b/mkfs.ubifs/mkfs.ubifs.c
@@ -20,6 +20,7 @@
  *          Zoltan Sogor
  */
 
+#define _XOPEN_SOURCE 500 /* For realpath() */
 #define PROGRAM_NAME "mkfs.ubifs"
 
 #include "mkfs.ubifs.h"
@@ -235,93 +236,50 @@ static char *make_path(const char *dir, const char *name)
 }
 
 /**
- * same_dir - determine if two file descriptors refer to the same directory.
- * @fd1: file descriptor 1
- * @fd2: file descriptor 2
- */
-static int same_dir(int fd1, int fd2)
-{
-	struct stat stat1, stat2;
-
-	if (fstat(fd1, &stat1) == -1)
-		return -1;
-	if (fstat(fd2, &stat2) == -1)
-		return -1;
-	return stat1.st_dev == stat2.st_dev && stat1.st_ino == stat2.st_ino;
-}
-
-/**
- * do_openat - open a file in a directory.
- * @fd: file descriptor of open directory
- * @path: path relative to directory
- * @flags: open flags
+ * is_contained - determine if a file is beneath a directory.
+ * @file: file path name
+ * @dir: directory path name
  *
- * This function is provided because the library function openat is sometimes
- * not available.
+ * This function returns %1 if @file is accessible from the @dir directory and
+ * %0 otherwise. In case of error, returns %-1.
  */
-static int do_openat(int fd, const char *path, int flags)
+static int is_contained(const char *file, const char *dir)
 {
-	int ret;
-	char *cwd;
+	char *file_base, *copy, *real_file, *real_dir, *p;
 
-	cwd = getcwd(NULL, 0);
-	if (!cwd)
+	/* Make a copy of the file path because 'dirname()' can modify it */
+	copy = strdup(file);
+	if (!copy)
 		return -1;
-	ret = fchdir(fd);
-	if (ret != -1)
-		ret = open(path, flags);
-	if (chdir(cwd) && !ret)
-		ret = -1;
-	free(cwd);
-	return ret;
-}
+	file_base = dirname(copy);
 
-/**
- * in_path - determine if a file is beneath a directory.
- * @dir_name: directory path name
- * @file_name: file path name
- */
-static int in_path(const char *dir_name, const char *file_name)
-{
-	char *fn = strdup(file_name);
-	char *dn;
-	int fd1, fd2, fd3, ret = -1, top_fd;
+	/* Turn the paths into the canonical form */
+	real_file = malloc(PATH_MAX);
+	if (!real_file) {
+		free(copy);
+		return -1;
+	}
 
-	if (!fn)
+	real_dir = malloc(PATH_MAX);
+	if (!real_dir) {
+		free(real_file);
+		free(copy);
+		return -1;
+	}
+	if (!realpath(file_base, real_file)) {
+		perror("realpath");
 		return -1;
-	top_fd = open("/", O_RDONLY);
-	if (top_fd != -1) {
-		dn = dirname(fn);
-		fd1 = open(dir_name, O_RDONLY);
-		if (fd1 != -1) {
-			fd2 = open(dn, O_RDONLY);
-			if (fd2 != -1) {
-				while (1) {
-					int same;
-
-					same = same_dir(fd1, fd2);
-					if (same) {
-						ret = same;
-						break;
-					}
-					if (same_dir(fd2, top_fd)) {
-						ret = 0;
-						break;
-					}
-					fd3 = do_openat(fd2, "..", O_RDONLY);
-					if (fd3 == -1)
-						break;
-					close(fd2);
-					fd2 = fd3;
-				}
-				close(fd2);
-			}
-			close(fd1);
-		}
-		close(top_fd);
 	}
-	free(fn);
-	return ret;
+	if (!realpath(dir, real_dir)) {
+		perror("realpath");
+		return -1;
+	}
+
+	p = strstr(real_file, real_dir);
+	free(real_dir);
+	free(real_file);
+	free(copy);
+	return !!p;
 }
 
 /**
@@ -376,7 +334,7 @@ static int validate_options(void)
 
 	if (!output)
 		return err_msg("no output file or UBI volume specified");
-	if (root && in_path(root, output))
+	if (root && is_contained(output, root))
 		return err_msg("output file cannot be in the UBIFS root "
 			       "directory");
 	if (!is_power_of_2(c->min_io_size))
-- 
1.7.11.4



-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #1.2: 0001-mkfs.ubifs-rewrite-path-checking.patch --]
[-- Type: text/x-patch, Size: 4879 bytes --]

From 7c1b939f2cf0141dfb1969d51b4157a75f55ddac Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Wed, 10 Oct 2012 14:23:18 +0300
Subject: [PATCH] mkfs.ubifs: rewrite path checking

We use the 'in_path()' function to check whether the output image is
withing the mkfs.ubifs root directory or not. However, this function
is not correct and it fails for the following situation, as
Marcus Prebble <marcus.prebble@axis.com> reports:

1. We have our root file-system mounted at / and want to build an image
   out of it.
2. We have tmpfs mounted at /tmp
3. We mount the root file-system under /tmp/newroot
4. We run mkfs.ubifs with -r /tmp/newroot -o /tmp/image

And this fails. It fails because 'in_path()' misses this use-case.

This patch re-implements the check completely. Now we use 'realpath()'
to find canonical paths and just check that the output file is not
under the root mkfs.ubifs directory.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 mkfs.ubifs/mkfs.ubifs.c | 116 +++++++++++++++---------------------------------
 1 file changed, 37 insertions(+), 79 deletions(-)

diff --git a/mkfs.ubifs/mkfs.ubifs.c b/mkfs.ubifs/mkfs.ubifs.c
index 149806b..885c202 100644
--- a/mkfs.ubifs/mkfs.ubifs.c
+++ b/mkfs.ubifs/mkfs.ubifs.c
@@ -20,6 +20,7 @@
  *          Zoltan Sogor
  */
 
+#define _XOPEN_SOURCE 500 /* For realpath() */
 #define PROGRAM_NAME "mkfs.ubifs"
 
 #include "mkfs.ubifs.h"
@@ -235,93 +236,50 @@ static char *make_path(const char *dir, const char *name)
 }
 
 /**
- * same_dir - determine if two file descriptors refer to the same directory.
- * @fd1: file descriptor 1
- * @fd2: file descriptor 2
- */
-static int same_dir(int fd1, int fd2)
-{
-	struct stat stat1, stat2;
-
-	if (fstat(fd1, &stat1) == -1)
-		return -1;
-	if (fstat(fd2, &stat2) == -1)
-		return -1;
-	return stat1.st_dev == stat2.st_dev && stat1.st_ino == stat2.st_ino;
-}
-
-/**
- * do_openat - open a file in a directory.
- * @fd: file descriptor of open directory
- * @path: path relative to directory
- * @flags: open flags
+ * is_contained - determine if a file is beneath a directory.
+ * @file: file path name
+ * @dir: directory path name
  *
- * This function is provided because the library function openat is sometimes
- * not available.
+ * This function returns %1 if @file is accessible from the @dir directory and
+ * %0 otherwise. In case of error, returns %-1.
  */
-static int do_openat(int fd, const char *path, int flags)
+static int is_contained(const char *file, const char *dir)
 {
-	int ret;
-	char *cwd;
+	char *file_base, *copy, *real_file, *real_dir, *p;
 
-	cwd = getcwd(NULL, 0);
-	if (!cwd)
+	/* Make a copy of the file path because 'dirname()' can modify it */
+	copy = strdup(file);
+	if (!copy)
 		return -1;
-	ret = fchdir(fd);
-	if (ret != -1)
-		ret = open(path, flags);
-	if (chdir(cwd) && !ret)
-		ret = -1;
-	free(cwd);
-	return ret;
-}
+	file_base = dirname(copy);
 
-/**
- * in_path - determine if a file is beneath a directory.
- * @dir_name: directory path name
- * @file_name: file path name
- */
-static int in_path(const char *dir_name, const char *file_name)
-{
-	char *fn = strdup(file_name);
-	char *dn;
-	int fd1, fd2, fd3, ret = -1, top_fd;
+	/* Turn the paths into the canonical form */
+	real_file = malloc(PATH_MAX);
+	if (!real_file) {
+		free(copy);
+		return -1;
+	}
 
-	if (!fn)
+	real_dir = malloc(PATH_MAX);
+	if (!real_dir) {
+		free(real_file);
+		free(copy);
+		return -1;
+	}
+	if (!realpath(file_base, real_file)) {
+		perror("realpath");
 		return -1;
-	top_fd = open("/", O_RDONLY);
-	if (top_fd != -1) {
-		dn = dirname(fn);
-		fd1 = open(dir_name, O_RDONLY);
-		if (fd1 != -1) {
-			fd2 = open(dn, O_RDONLY);
-			if (fd2 != -1) {
-				while (1) {
-					int same;
-
-					same = same_dir(fd1, fd2);
-					if (same) {
-						ret = same;
-						break;
-					}
-					if (same_dir(fd2, top_fd)) {
-						ret = 0;
-						break;
-					}
-					fd3 = do_openat(fd2, "..", O_RDONLY);
-					if (fd3 == -1)
-						break;
-					close(fd2);
-					fd2 = fd3;
-				}
-				close(fd2);
-			}
-			close(fd1);
-		}
-		close(top_fd);
 	}
-	free(fn);
-	return ret;
+	if (!realpath(dir, real_dir)) {
+		perror("realpath");
+		return -1;
+	}
+
+	p = strstr(real_file, real_dir);
+	free(real_dir);
+	free(real_file);
+	free(copy);
+	return !!p;
 }
 
 /**
@@ -376,7 +334,7 @@ static int validate_options(void)
 
 	if (!output)
 		return err_msg("no output file or UBI volume specified");
-	if (root && in_path(root, output))
+	if (root && is_contained(output, root))
 		return err_msg("output file cannot be in the UBIFS root "
 			       "directory");
 	if (!is_power_of_2(c->min_io_size))
-- 
1.7.11.4


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Marcus Prebble]

[-- Attachment #1: Type: text/plain, Size: 5941 bytes --]

Hi Artem,

Big thanks for the provided patch. It seems to solve the problem we here
at Axis and other users were having.
When trying to break it (unsuccessfully) I found that if the directory
of the output file was non-existent then the function would return -1
(correct), but the error messages were:

"realpath: no such file or directory"
"Error: output file cannot be in the UBIFS root directory"

which was a tad confusing. Please find attached a new diff (based off
yours) with a few minor suggested changes.

Kind regards,

/Marcus Prebble



On Wed, 2012-10-10 at 13:30 +0200, Artem Bityutskiy wrote:
> On Wed, 2012-10-10 at 12:34 +0200, Marcus Prebble wrote:
> > I will implement and come back with a patch when I have time.
> 
> I actually went ahead and cooked something. Could you please review and
> give it a test? Inlined below and also attached. Thanks!
> 
> 
> From 7c1b939f2cf0141dfb1969d51b4157a75f55ddac Mon Sep 17 00:00:00 2001
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Date: Wed, 10 Oct 2012 14:23:18 +0300
> Subject: [PATCH] mkfs.ubifs: rewrite path checking
> 
> We use the 'in_path()' function to check whether the output image is
> withing the mkfs.ubifs root directory or not. However, this function
> is not correct and it fails for the following situation, as
> Marcus Prebble <marcus.prebble@axis.com> reports:
> 
> 1. We have our root file-system mounted at / and want to build an image
>    out of it.
> 2. We have tmpfs mounted at /tmp
> 3. We mount the root file-system under /tmp/newroot
> 4. We run mkfs.ubifs with -r /tmp/newroot -o /tmp/image
> 
> And this fails. It fails because 'in_path()' misses this use-case.
> 
> This patch re-implements the check completely. Now we use 'realpath()'
> to find canonical paths and just check that the output file is not
> under the root mkfs.ubifs directory.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  mkfs.ubifs/mkfs.ubifs.c | 116 +++++++++++++++---------------------------------
>  1 file changed, 37 insertions(+), 79 deletions(-)
> 
> diff --git a/mkfs.ubifs/mkfs.ubifs.c b/mkfs.ubifs/mkfs.ubifs.c
> index 149806b..885c202 100644
> --- a/mkfs.ubifs/mkfs.ubifs.c
> +++ b/mkfs.ubifs/mkfs.ubifs.c
> @@ -20,6 +20,7 @@
>   *          Zoltan Sogor
>   */
>  
> +#define _XOPEN_SOURCE 500 /* For realpath() */
>  #define PROGRAM_NAME "mkfs.ubifs"
>  
>  #include "mkfs.ubifs.h"
> @@ -235,93 +236,50 @@ static char *make_path(const char *dir, const char *name)
>  }
>  
>  /**
> - * same_dir - determine if two file descriptors refer to the same directory.
> - * @fd1: file descriptor 1
> - * @fd2: file descriptor 2
> - */
> -static int same_dir(int fd1, int fd2)
> -{
> -	struct stat stat1, stat2;
> -
> -	if (fstat(fd1, &stat1) == -1)
> -		return -1;
> -	if (fstat(fd2, &stat2) == -1)
> -		return -1;
> -	return stat1.st_dev == stat2.st_dev && stat1.st_ino == stat2.st_ino;
> -}
> -
> -/**
> - * do_openat - open a file in a directory.
> - * @fd: file descriptor of open directory
> - * @path: path relative to directory
> - * @flags: open flags
> + * is_contained - determine if a file is beneath a directory.
> + * @file: file path name
> + * @dir: directory path name
>   *
> - * This function is provided because the library function openat is sometimes
> - * not available.
> + * This function returns %1 if @file is accessible from the @dir directory and
> + * %0 otherwise. In case of error, returns %-1.
>   */
> -static int do_openat(int fd, const char *path, int flags)
> +static int is_contained(const char *file, const char *dir)
>  {
> -	int ret;
> -	char *cwd;
> +	char *file_base, *copy, *real_file, *real_dir, *p;
>  
> -	cwd = getcwd(NULL, 0);
> -	if (!cwd)
> +	/* Make a copy of the file path because 'dirname()' can modify it */
> +	copy = strdup(file);
> +	if (!copy)
>  		return -1;
> -	ret = fchdir(fd);
> -	if (ret != -1)
> -		ret = open(path, flags);
> -	if (chdir(cwd) && !ret)
> -		ret = -1;
> -	free(cwd);
> -	return ret;
> -}
> +	file_base = dirname(copy);
>  
> -/**
> - * in_path - determine if a file is beneath a directory.
> - * @dir_name: directory path name
> - * @file_name: file path name
> - */
> -static int in_path(const char *dir_name, const char *file_name)
> -{
> -	char *fn = strdup(file_name);
> -	char *dn;
> -	int fd1, fd2, fd3, ret = -1, top_fd;
> +	/* Turn the paths into the canonical form */
> +	real_file = malloc(PATH_MAX);
> +	if (!real_file) {
> +		free(copy);
> +		return -1;
> +	}
>  
> -	if (!fn)
> +	real_dir = malloc(PATH_MAX);
> +	if (!real_dir) {
> +		free(real_file);
> +		free(copy);
> +		return -1;
> +	}
> +	if (!realpath(file_base, real_file)) {
> +		perror("realpath");
>  		return -1;
> -	top_fd = open("/", O_RDONLY);
> -	if (top_fd != -1) {
> -		dn = dirname(fn);
> -		fd1 = open(dir_name, O_RDONLY);
> -		if (fd1 != -1) {
> -			fd2 = open(dn, O_RDONLY);
> -			if (fd2 != -1) {
> -				while (1) {
> -					int same;
> -
> -					same = same_dir(fd1, fd2);
> -					if (same) {
> -						ret = same;
> -						break;
> -					}
> -					if (same_dir(fd2, top_fd)) {
> -						ret = 0;
> -						break;
> -					}
> -					fd3 = do_openat(fd2, "..", O_RDONLY);
> -					if (fd3 == -1)
> -						break;
> -					close(fd2);
> -					fd2 = fd3;
> -				}
> -				close(fd2);
> -			}
> -			close(fd1);
> -		}
> -		close(top_fd);
>  	}
> -	free(fn);
> -	return ret;
> +	if (!realpath(dir, real_dir)) {
> +		perror("realpath");
> +		return -1;
> +	}
> +
> +	p = strstr(real_file, real_dir);
> +	free(real_dir);
> +	free(real_file);
> +	free(copy);
> +	return !!p;
>  }
>  
>  /**
> @@ -376,7 +334,7 @@ static int validate_options(void)
>  
>  	if (!output)
>  		return err_msg("no output file or UBI volume specified");
> -	if (root && in_path(root, output))
> +	if (root && is_contained(output, root))
>  		return err_msg("output file cannot be in the UBIFS root "
>  			       "directory");
>  	if (!is_power_of_2(c->min_io_size))
> -- 
> 1.7.11.4
> 
> 
> 


[-- Attachment #2: 0002-mkfs.ubifs-rewrite-path-checking.patch --]
[-- Type: text/x-patch, Size: 2088 bytes --]

diff --git a/mtd-utils/mkfs.ubifs/mkfs.ubifs.c b/mtd-utils/mkfs.ubifs/mkfs.ubifs.c
index a361df9..dce21ae 100644
--- a/mtd-utils/mkfs.ubifs/mkfs.ubifs.c
+++ b/mtd-utils/mkfs.ubifs/mkfs.ubifs.c
@@ -252,7 +252,10 @@ static char *make_path(const char *dir, const char *name)
  */
 static int is_contained(const char *file, const char *dir)
 {
-	char *file_base, *copy, *real_file, *real_dir, *p;
+	char *real_file = NULL;
+	char *real_dir = NULL;
+	char *file_base, *copy;
+	int ret = -1;
 
 	/* Make a copy of the file path because 'dirname()' can modify it */
 	copy = strdup(file);
@@ -262,31 +265,29 @@ static int is_contained(const char *file, const char *dir)
 
 	/* Turn the paths into the canonical form */
 	real_file = malloc(PATH_MAX);
-	if (!real_file) {
-		free(copy);
-		return -1;
-	}
+	if (!real_file)
+		goto out_free;
 
 	real_dir = malloc(PATH_MAX);
-	if (!real_dir) {
-		free(real_file);
-		free(copy);
-		return -1;
-	}
+	if (!real_dir)
+		goto out_free;
+
 	if (!realpath(file_base, real_file)) {
-		perror("realpath");
-		return -1;
+		perror("realpath file");
+		goto out_free;
 	}
 	if (!realpath(dir, real_dir)) {
-		perror("realpath");
-		return -1;
+		perror("realpath directory");
+		goto out_free;
 	}
 
-	p = strstr(real_file, real_dir);
-	free(real_dir);
-	free(real_file);
+	ret = strstr(real_file, real_dir) ? 1 : 0;
+
+out_free:
 	free(copy);
-	return !!p;
+	free(real_file);
+	free(real_dir);
+	return ret;
 }
 
 /**
@@ -346,9 +347,13 @@ static int validate_options(void)
 
 	if (!output)
 		return err_msg("no output file or UBI volume specified");
-	if (root && is_contained(output, root))
-		return err_msg("output file cannot be in the UBIFS root "
-			       "directory");
+	if (root) {
+		if ((tmp = is_contained(output, root)) < 0)
+			return err_msg("failed to perform output file root check");
+		else if (tmp)
+			return err_msg("output file cannot be in the UBIFS root "
+			               "directory");
+	}
 	if (!is_power_of_2(c->min_io_size))
 		return err_msg("min. I/O unit size should be power of 2");
 	if (c->leb_size < c->min_io_size)

Re: mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 394 bytes --]

On Wed, 2012-10-10 at 17:26 +0200, Marcus Prebble wrote:
> which was a tad confusing. Please find attached a new diff (based off
> yours) with a few minor suggested changes.

Do you want me to fold your patch in? Or you prefer to keep it as a
separate patch? In the latter case, please, send it as a patch, with
commit message, and signed-off-by.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: mkfs.ubifs problem when output file really isn't in the UBIFS root directory

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

On Wed, 2012-10-10 at 17:26 +0200, Marcus Prebble wrote:
> which was a tad confusing. Please find attached a new diff (based off
> yours) with a few minor suggested changes.

Actually, I've just pushed my patch out to mtd-utils.git. Would you
please, send your diff as a normal patch on top of that?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]