Re: [PATCH] udf: Fix regression which is preventing symlink access

[Daniel Kiper]

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 <development@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