On Fri, Sep 10, 2021 at 04:03:23PM +0000, Glenn Washburn wrote:
> This code was broken by commit 3f05d693 ("malloc: Use overflow checking
> primitives where we do complex allocations"), which added overflow checking
> in many areas. The problem here is that the changes update the local
> variable sz, which was already in use and which was not updated before the
> change. So the code using sz was getting a different value of than it would
> have previously for the same UDF image. This causes the logic getting the
> destination of the symlink to not realize that its gotten the full
> destination, but keeps trying to read past the end of the destination. The
> bytes after the end are generally NULL padding bytes, but that's not a
> valid component type (ECMA-167 14.16.1.1). So grub_udf_read_symlink branches
> to error logic, returning NULL, instead of the symlink destination path.
>
> The result of this bug is that the UDF filesystem tests were failing in the
> symlink test with the grub-fstest error message:
>
> grub-fstest: error: cannot open `(loop0)/sym': invalid symlink.
>
> This change stores the result of doubling sz in another local variable s,
> so as not to modify sz. Also remove unnecessary grub_add, which increased
> the output by 1 to account for a NULL byte. This isn't needed because an
> output buffer of size twice sz is already guaranteed to be more than enough
> to contain the path components converted to UTF-8. The worst case upper-
> bound for the needed output buffer size is (sz-4)*1.5, where 4 is the size
I think 4 comes from ECMA-167 spec. Could you add a reference to it here?
The number of paragraph would be perfect...
> of a path component header and 1.5 is the maximum growth in bytes when
> converting from 2-byte unicode code-points to UTF-8 (from 2 bytes to 3).
Could you explain how did you come up with the 1.5 value? It would be
nice if you refer to a spec or something like that.
Daniel
> Signed-off-by: Glenn Washburn <developm...@efficientek.com>
> ---
> grub-core/fs/udf.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c
> index 2ac5c1d00..197e60d12 100644
> --- a/grub-core/fs/udf.c
> +++ b/grub-core/fs/udf.c
> @@ -1022,7 +1022,7 @@ grub_udf_iterate_dir (grub_fshelp_node_t dir,
> static char *
> grub_udf_read_symlink (grub_fshelp_node_t node)
> {
> - grub_size_t sz = U64 (node->block.fe.file_size);
> + grub_size_t s, sz = U64 (node->block.fe.file_size);
> grub_uint8_t *raw;
> const grub_uint8_t *ptr;
> char *out = NULL, *optr;
> @@ -1035,11 +1035,19 @@ grub_udf_read_symlink (grub_fshelp_node_t node)
> if (grub_udf_read_file (node, NULL, NULL, 0, sz, (char *) raw) < 0)
> goto fail_1;
>
> - if (grub_mul (sz, 2, &sz) ||
> - grub_add (sz, 1, &sz))
> + /*
> + * Local sz is the size of the symlink file data, which contains a sequence
> + * of path components (ECMA-167 14.16.1) representing the link destination.
> + * This size is an upper-bound on the number of bytes of a contained and
> + * potentially compressed 2-byte character string. Allocate 2*sz for the
> + * output buffer contained the string converted to UTF-8 because the
> + * resulting string can not be more than double the size (2-byte unicode
> + * points will be converted to a maximum of 3 bytes in UTF-8).
> + */
> + if (grub_mul (sz, 2, &s))
> goto fail_0;
>
> - out = grub_malloc (sz);
> + out = grub_malloc (s);
> if (!out)
> {
> fail_0:
> @@ -1051,7 +1059,6 @@ grub_udf_read_symlink (grub_fshelp_node_t node)
>
> for (ptr = raw; ptr < raw + sz; )
> {
> - grub_size_t s;
> if ((grub_size_t) (ptr - raw + 4) > sz)
> goto fail_1;
> if (!(ptr[2] == 0 && ptr[3] == 0))
> --
> 2.32.0
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
