Re: [PATCH] hurd: Protect against servers returning bogus read/write lengths

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le mer. 04 déc. 2024 14:29:15 +0300, a ecrit:
> There already was a branch checking for this case in _hurd_fd_read ()
> when the data is returned out-of-line. Do the same for inline data, as
> well as for _hurd_fd_write (). It's also not possible for the length to
> be negative, since it's stored in an unsigned integer.
> 
> Not verifying the returned length can confuse the callers who assume
> the returned length is always reasonable. This manifested as libzstd
> test suite failing on writes to /dev/zero, even though the write () call
> appeared to succeed. In fact, the zero store backing /dev/zero was
> returning a larger written length than the size actually submitted to
> it, which is a separate bug to be fixed on the Hurd side. With this
> patch, EGRATUITOUS is now propagated to the caller.
> 
> Reported-by: Diego Nieto Cid <dnietoc@gmail.com>
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/fd-read.c  | 12 +++++++-----
>  hurd/fd-write.c | 10 +++++++---
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/hurd/fd-read.c b/hurd/fd-read.c
> index 7ce5d5bde..e492c3288 100644
> --- a/hurd/fd-read.c
> +++ b/hurd/fd-read.c
> @@ -38,13 +38,15 @@ _hurd_fd_read (struct hurd_fd *fd, void *buf, size_t 
> *nbytes, loff_t offset)
>    if (err = HURD_FD_PORT_USE_CANCEL (fd, _hurd_ctty_input (port, ctty, 
> readfd)))
>      return err;
>  
> +  if (__glibc_unlikely (nread > *nbytes))    /* Sanity check for bogus 
> server.  */
> +    {
> +      if (data != buf)
> +     __vm_deallocate (__mach_task_self (), (vm_address_t) data, nread);
> +      return EGRATUITOUS;
> +    }
> +
>    if (data != buf)
>      {
> -      if (nread > *nbytes)   /* Sanity check for bogus server.  */
> -     {
> -       __vm_deallocate (__mach_task_self (), (vm_address_t) data, nread);
> -       return EGRATUITOUS;
> -     }
>        memcpy (buf, data, nread);
>        __vm_deallocate (__mach_task_self (), (vm_address_t) data, nread);
>      }
> diff --git a/hurd/fd-write.c b/hurd/fd-write.c
> index 797880756..2f070c5c5 100644
> --- a/hurd/fd-write.c
> +++ b/hurd/fd-write.c
> @@ -34,9 +34,13 @@ _hurd_fd_write (struct hurd_fd *fd,
>      }
>  
>    err = HURD_FD_PORT_USE_CANCEL (fd, _hurd_ctty_output (port, ctty, 
> writefd));
> +  if (err)
> +    return err;
>  
> -  if (! err)
> -    *nbytes = wrote;
> +  if (__glibc_unlikely (wrote > *nbytes))    /* Sanity check for bogus 
> server.  */
> +    return EGRATUITOUS;
>  
> -  return err;
> +  *nbytes = wrote;
> +
> +  return 0;
>  }
> -- 
> 2.47.1
> 
> 

-- 
Samuel
#include <culture.h>