[U-Boot] [PATCH] fat: handle paths that include ../

[U-Boot] [PATCH] fat: handle paths that include ../

[Stephen Warren]

The FAT code contains a special case to parse the root directory. This
is needed since the root directory location/layout on disk is special
cased for FAT12/16. In particular, the location and size of the FAT12/16
root directory is hard-coded and contiguous, whereas all FAT12/16 non-root
directories, and all FAT32 directories, are stored in a non-contiguous
fashion, with the layout represented by a linked-list of clusters in the
FAT.

If a file path contains ../ (for example /extlinux/../bcm2835-rpi-cm.dtb),
it is possible to need to parse the root directory for the first element
in the path (requiring application of the special case), then a sub-
directory (in the general way), then re-parse the root directory (again
requiring the special case). However, the current code in U-Boot only
applies the special case for the very first path element, and never for
any later path element. When reparsing the root directory without
applying the special case, any file in a sector (or cluster?) other than
the first sector/cluster of the root directory will not be found.

This change modifies the non-root-dir-parsing loop of do_fat_read_at()
to detect if it's walked back to the root directory, and if so, jumps
back to the special case code that handles parsing of the root directory.

This change was tested using sandbox by executing:

./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0"
./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /"
./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux"
./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/"
./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/.."
./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../"
./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup"
./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup/"
./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup/.."
./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup/../"
./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /bcm2835-rpi-cm.dtb"
./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /extlinux/../bcm2835-rpi-cm.dtb"
./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /backup/../bcm2835-rpi-cm.dtb"
./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /extlinux/..backup/../bcm2835-rpi-cm.dtb"
./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /extlinux/../backup/../bcm2835-rpi-cm.dtb"

(/extlinux and /backup are in different sectors so trigger some different
cases, and bcm2835-rpi-cm.dtb is in a sector of the root directory other
than the first).

In all honesty, this change is a bit of a hack, using goto and all.
However, as demonstrated above it appears to work well in practice, is
quite minimal, likely doesn't introduce any risk of regressions, and
hopefully doesn't introduce any maintenance issues.

The correct fix would be to collapse the root and non-root loops in
do_fat_read_at() and get_dentfromdir() into a single loop that has a
small special-case when moving from one sector to the next, to handle
the layout difference of root/non-root directories. AFAIK all other
aspects of directory parsing are identical. However, that's a much
larger change which needs significantly more thought before it's
implemented.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
 fs/fat/fat.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index bccc3e3ed8fd..a863644d6480 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -895,6 +895,7 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 	strcpy(fnamecopy, filename);
 	downcase(fnamecopy);
 
+root_reparse:
 	if (*fnamecopy == '\0') {
 		if (!dols)
 			goto exit;
@@ -1180,6 +1181,34 @@ rootdir_done:
 		if (isdir && !(dentptr->attr & ATTR_DIR))
 			goto exit;
 
+		/*
+		 * If we are looking for a directory, and found a directory
+		 * type entry, and the entry is for the root directory (as
+		 * denoted by a cluster number of 0), jump back to the start
+		 * of the function, since at least on FAT12/16, the root dir
+		 * lives in a hard-coded location and needs special handling
+		 * to parse, rather than simply following the cluster linked
+		 * list in the FAT, like other directories.
+		 */
+		if (isdir && (dentptr->attr & ATTR_DIR) && !START(dentptr)) {
+			/*
+			 * Modify the filename to remove the prefix that gets
+			 * back to the root directory, so the initial root dir
+			 * parsing code can continue from where we are without
+			 * confusion.
+			 */
+			strcpy(fnamecopy, nextname ?: "");
+			/*
+			 * Set up state the same way as the function does when
+			 * first started. This is required for the root dir
+			 * parsing code operates in its expected environment.
+			 */
+			subname = "";
+			cursect = mydata->rootdir_sect;
+			isdir = 0;
+			goto root_reparse;
+		}
+
 		if (idx >= 0)
 			subname = nextname;
 	}
-- 
1.9.1

[U-Boot] [PATCH] fat: handle paths that include ../

[Stephen Warren]

On 07/28/2015 08:55 PM, Stephen Warren wrote:
> The FAT code contains a special case to parse the root directory. This
> is needed since the root directory location/layout on disk is special
> cased for FAT12/16. In particular, the location and size of the FAT12/16
> root directory is hard-coded and contiguous, whereas all FAT12/16 non-root
> directories, and all FAT32 directories, are stored in a non-contiguous
> fashion, with the layout represented by a linked-list of clusters in the
> FAT.
> 
> If a file path contains ../ (for example /extlinux/../bcm2835-rpi-cm.dtb),
> it is possible to need to parse the root directory for the first element
> in the path (requiring application of the special case), then a sub-
> directory (in the general way), then re-parse the root directory (again
> requiring the special case). However, the current code in U-Boot only
> applies the special case for the very first path element, and never for
> any later path element. When reparsing the root directory without
> applying the special case, any file in a sector (or cluster?) other than
> the first sector/cluster of the root directory will not be found.
> 
> This change modifies the non-root-dir-parsing loop of do_fat_read_at()
> to detect if it's walked back to the root directory, and if so, jumps
> back to the special case code that handles parsing of the root directory.

Is this change slated for v2015.10, or is the plan to leave this issue
in place until the FAT implementation replacement is accepted for the
release after that?

> 
> This change was tested using sandbox by executing:
> 
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/.."
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup/"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup/.."
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup/../"
> ./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /bcm2835-rpi-cm.dtb"
> ./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /extlinux/../bcm2835-rpi-cm.dtb"
> ./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /backup/../bcm2835-rpi-cm.dtb"
> ./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /extlinux/..backup/../bcm2835-rpi-cm.dtb"
> ./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /extlinux/../backup/../bcm2835-rpi-cm.dtb"
> 
> (/extlinux and /backup are in different sectors so trigger some different
> cases, and bcm2835-rpi-cm.dtb is in a sector of the root directory other
> than the first).
> 
> In all honesty, this change is a bit of a hack, using goto and all.
> However, as demonstrated above it appears to work well in practice, is
> quite minimal, likely doesn't introduce any risk of regressions, and
> hopefully doesn't introduce any maintenance issues.
> 
> The correct fix would be to collapse the root and non-root loops in
> do_fat_read_at() and get_dentfromdir() into a single loop that has a
> small special-case when moving from one sector to the next, to handle
> the layout difference of root/non-root directories. AFAIK all other
> aspects of directory parsing are identical. However, that's a much
> larger change which needs significantly more thought before it's
> implemented.

[U-Boot] [PATCH] fat: handle paths that include ../

[Tom Rini]

On Thu, Sep 10, 2015 at 10:22:35AM -0700, Stephen Warren wrote:
> On 07/28/2015 08:55 PM, Stephen Warren wrote:
> > The FAT code contains a special case to parse the root directory. This
> > is needed since the root directory location/layout on disk is special
> > cased for FAT12/16. In particular, the location and size of the FAT12/16
> > root directory is hard-coded and contiguous, whereas all FAT12/16 non-root
> > directories, and all FAT32 directories, are stored in a non-contiguous
> > fashion, with the layout represented by a linked-list of clusters in the
> > FAT.
> > 
> > If a file path contains ../ (for example /extlinux/../bcm2835-rpi-cm.dtb),
> > it is possible to need to parse the root directory for the first element
> > in the path (requiring application of the special case), then a sub-
> > directory (in the general way), then re-parse the root directory (again
> > requiring the special case). However, the current code in U-Boot only
> > applies the special case for the very first path element, and never for
> > any later path element. When reparsing the root directory without
> > applying the special case, any file in a sector (or cluster?) other than
> > the first sector/cluster of the root directory will not be found.
> > 
> > This change modifies the non-root-dir-parsing loop of do_fat_read_at()
> > to detect if it's walked back to the root directory, and if so, jumps
> > back to the special case code that handles parsing of the root directory.
> 
> Is this change slated for v2015.10, or is the plan to leave this issue
> in place until the FAT implementation replacement is accepted for the
> release after that?

I believe I shall grab this.  Did you see the email from Lukasz saying
the new FAT didn't pass the DFU test suite?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150910/6e2f7cc6/attachment.sig>

[U-Boot] [PATCH] fat: handle paths that include ../

[Stephen Warren]

On 09/10/2015 03:26 PM, Tom Rini wrote:
> On Thu, Sep 10, 2015 at 10:22:35AM -0700, Stephen Warren wrote:
>> On 07/28/2015 08:55 PM, Stephen Warren wrote:
>>> The FAT code contains a special case to parse the root
>>> directory. This is needed since the root directory
>>> location/layout on disk is special cased for FAT12/16. In
>>> particular, the location and size of the FAT12/16 root
>>> directory is hard-coded and contiguous, whereas all FAT12/16
>>> non-root directories, and all FAT32 directories, are stored in
>>> a non-contiguous fashion, with the layout represented by a
>>> linked-list of clusters in the FAT.
>>> 
>>> If a file path contains ../ (for example
>>> /extlinux/../bcm2835-rpi-cm.dtb), it is possible to need to
>>> parse the root directory for the first element in the path
>>> (requiring application of the special case), then a sub- 
>>> directory (in the general way), then re-parse the root
>>> directory (again requiring the special case). However, the
>>> current code in U-Boot only applies the special case for the
>>> very first path element, and never for any later path element.
>>> When reparsing the root directory without applying the special
>>> case, any file in a sector (or cluster?) other than the first
>>> sector/cluster of the root directory will not be found.
>>> 
>>> This change modifies the non-root-dir-parsing loop of
>>> do_fat_read_at() to detect if it's walked back to the root
>>> directory, and if so, jumps back to the special case code that
>>> handles parsing of the root directory.
>> 
>> Is this change slated for v2015.10, or is the plan to leave this
>> issue in place until the FAT implementation replacement is
>> accepted for the release after that?
> 
> I believe I shall grab this.  Did you see the email from Lukasz
> saying the new FAT didn't pass the DFU test suite?

Yes. I'll try testing that when I get back from travel.

[U-Boot] fat: handle paths that include ../

[Tom Rini]

On Tue, Jul 28, 2015 at 09:55:03PM -0600, Stephen Warren wrote:

> The FAT code contains a special case to parse the root directory. This
> is needed since the root directory location/layout on disk is special
> cased for FAT12/16. In particular, the location and size of the FAT12/16
> root directory is hard-coded and contiguous, whereas all FAT12/16 non-root
> directories, and all FAT32 directories, are stored in a non-contiguous
> fashion, with the layout represented by a linked-list of clusters in the
> FAT.
> 
> If a file path contains ../ (for example /extlinux/../bcm2835-rpi-cm.dtb),
> it is possible to need to parse the root directory for the first element
> in the path (requiring application of the special case), then a sub-
> directory (in the general way), then re-parse the root directory (again
> requiring the special case). However, the current code in U-Boot only
> applies the special case for the very first path element, and never for
> any later path element. When reparsing the root directory without
> applying the special case, any file in a sector (or cluster?) other than
> the first sector/cluster of the root directory will not be found.
> 
> This change modifies the non-root-dir-parsing loop of do_fat_read_at()
> to detect if it's walked back to the root directory, and if so, jumps
> back to the special case code that handles parsing of the root directory.
> 
> This change was tested using sandbox by executing:
> 
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/.."
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup/"
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup/.."
> ./u-boot -c "host bind 0 ../sd-p1.bin; ls host 0:0 /extlinux/../backup/../"
> ./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /bcm2835-rpi-cm.dtb"
> ./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /extlinux/../bcm2835-rpi-cm.dtb"
> ./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /backup/../bcm2835-rpi-cm.dtb"
> ./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /extlinux/..backup/../bcm2835-rpi-cm.dtb"
> ./u-boot -c "host bind 0 ../sd-p1.bin; load host 0:0 0 /extlinux/../backup/../bcm2835-rpi-cm.dtb"
> 
> (/extlinux and /backup are in different sectors so trigger some different
> cases, and bcm2835-rpi-cm.dtb is in a sector of the root directory other
> than the first).
> 
> In all honesty, this change is a bit of a hack, using goto and all.
> However, as demonstrated above it appears to work well in practice, is
> quite minimal, likely doesn't introduce any risk of regressions, and
> hopefully doesn't introduce any maintenance issues.
> 
> The correct fix would be to collapse the root and non-root loops in
> do_fat_read_at() and get_dentfromdir() into a single loop that has a
> small special-case when moving from one sector to the next, to handle
> the layout difference of root/non-root directories. AFAIK all other
> aspects of directory parsing are identical. However, that's a much
> larger change which needs significantly more thought before it's
> implemented.
> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150912/07e66589/attachment.sig>