[v2] malloc: Do not clobber errno on __getrandom_nocancel (BZ#29624)

  Use INTERNAL_SYSCALL_CALL instead of INLINE_SYSCALL_CALL.  This
requires emulate the semantic for hurd call (so __arc4random_buf
uses the fallback).

Checked on x86_64-linux-gnu.
---
 stdlib/arc4random.c                  |  4 ++--
 sysdeps/mach/hurd/not-cancel.h       | 12 ++++++++++--
 sysdeps/unix/sysv/linux/not-cancel.h |  2 +-
 3 files changed, 13 insertions(+), 5 deletions(-)
  

Hi Adhemerval,

Another question, the syscall is defined as:

ssize_t getrandom (void *__buffer, size_t __length,

Doesn't this mean if we use 'int' for the return value, a large but valid syscall
result could be interpreted as a negative error value? It sounds like all code
processing the getrandom syscall should use ssize_t rather than int. Or do we
limit length to something fairly small?

 __arc4random_buf (void *p, size_t n)
 {
   static int seen_initialized;
-  size_t l;
+  int l;

Should be ssize_t?

+static inline int
+__getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)

ssize_t?

+{
+  int save_errno = errno;
+  int r = __getrandom (buf, buflen, flags);

ssize_t?

+  r = r == -1 ? -errno : r;
+  __set_errno (save_errno);
+  return r;
+}
 
 #define __poll_infinity_nocancel(fds, nfds) \
   __poll (fds, nfds, -1)
diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
index a263d294b1..00ab75a405 100644
--- a/sysdeps/unix/sysv/linux/not-cancel.h
+++ b/sysdeps/unix/sysv/linux/not-cancel.h
@@ -71,7 +71,7 @@ __writev_nocancel_nostatus (int fd, const struct iovec *iov, int iovcnt)
 static inline int

ssize_t?

 __getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
 {
-  return INLINE_SYSCALL_CALL (getrandom, buf, buflen, flags);
+  return INTERNAL_SYSCALL_CALL (getrandom, buf, buflen, flags);
 }

Cheers,
Wilco
  

Hi,

29 septembre 2022 à 19:56 "Adhemerval Zanella via Libc-alpha" a écrit:

> 
> Use INTERNAL_SYSCALL_CALL instead of INLINE_SYSCALL_CALL. This
> requires emulate the semantic for hurd call (so __arc4random_buf
> uses the fallback).
> 
> Checked on x86_64-linux-gnu.
> ---
>  stdlib/arc4random.c | 4 ++--
>  sysdeps/mach/hurd/not-cancel.h | 12 ++++++++++--
>  sysdeps/unix/sysv/linux/not-cancel.h | 2 +-
>  3 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/stdlib/arc4random.c b/stdlib/arc4random.c
> index e417ef624d..0a1f4b8a57 100644
> --- a/stdlib/arc4random.c
> +++ b/stdlib/arc4random.c
> @@ -34,7 +34,7 @@ void
>  __arc4random_buf (void *p, size_t n)
>  {
>  static int seen_initialized;
> - size_t l;
> + int l;
>  int fd;
>  
>  if (n == 0)
> @@ -51,7 +51,7 @@ __arc4random_buf (void *p, size_t n)
>  n -= l;
>  continue; /* Interrupted by a signal; keep going. */
>  }
> - else if (l < 0 && errno == ENOSYS)

Aside: I failed to noticed this (the fact is, except commit message, I didn't review much Jason's patch) .

https://sourceware.org/pipermail/libc-alpha/2022-July/141049.html

l being size_t, it cannot be less than 0, thus is there a chance it can actually fallback to reading /dev/urandom ?

> + else if (l == -ENOSYS)
>  break; /* No syscall, so fallback to /dev/urandom. */
>  arc4random_getrandom_failure ();
>  }
> diff --git a/sysdeps/mach/hurd/not-cancel.h b/sysdeps/mach/hurd/not-cancel.h
> index ae58b734e3..518f738519 100644
> --- a/sysdeps/mach/hurd/not-cancel.h
> +++ b/sysdeps/mach/hurd/not-cancel.h
> @@ -27,6 +27,7 @@
>  #include <sys/uio.h>
>  #include <hurd.h>
>  #include <hurd/fd.h>
> +#include <sys/random.h>
>  
>  /* Non cancellable close syscall. */
>  __typeof (__close) __close_nocancel;
> @@ -75,8 +76,15 @@ __typeof (__fcntl) __fcntl_nocancel;
>  #define __fcntl64_nocancel(...) \
>  __fcntl_nocancel (__VA_ARGS__)
>  
> -#define __getrandom_nocancel(buf, size, flags) \
> - __getrandom (buf, size, flags)
> +static inline int
> +__getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
> +{
> + int save_errno = errno;
> + int r = __getrandom (buf, buflen, flags);
> + r = r == -1 ? -errno : r;
> + __set_errno (save_errno);
> + return r;
> +}
>  
>  #define __poll_infinity_nocancel(fds, nfds) \
>  __poll (fds, nfds, -1)
> diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
> index a263d294b1..00ab75a405 100644
> --- a/sysdeps/unix/sysv/linux/not-cancel.h
> +++ b/sysdeps/unix/sysv/linux/not-cancel.h
> @@ -71,7 +71,7 @@ __writev_nocancel_nostatus (int fd, const struct iovec *iov, int iovcnt)
>  static inline int
>  __getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
>  {
> - return INLINE_SYSCALL_CALL (getrandom, buf, buflen, flags);
> + return INTERNAL_SYSCALL_CALL (getrandom, buf, buflen, flags);
>  }
>  
>  static inline int
> -- 
> 2.34.1
>

Reviewed-by: Yann Droneaud <ydroneaud@opteya.com>
  

On 29/09/22 15:36, Wilco Dijkstra wrote:
> Hi Adhemerval,
> 
> Another question, the syscall is defined as:
> 
> ssize_t getrandom (void *__buffer, size_t __length,
> 
> Doesn't this mean if we use 'int' for the return value, a large but valid syscall
> result could be interpreted as a negative error value? It sounds like all code
> processing the getrandom syscall should use ssize_t rather than int. Or do we
> limit length to something fairly small?

Yeah, you are right.  The syscall indeed returns ssize_t/long:

include/linux/syscalls.h:1007:asmlinkage long sys_getrandom(char __user *buf, size_t count,

So it does make sense to use ssize_t. It seems that not all architectures handle
INTERNAL_SYSCALL consistently to use 'long', but at least the 64 bits does.

It also handles the issue raised by Yann, where arc4random fallback is not used. 
This is in fact another issue and I will send an independently patch. 

> 
>  __arc4random_buf (void *p, size_t n)
>  {
>    static int seen_initialized;
> -  size_t l;
> +  int l;
> 
> Should be ssize_t?
> 
> +static inline int
> +__getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
> 
> ssize_t?
> 
> +{
> +  int save_errno = errno;
> +  int r = __getrandom (buf, buflen, flags);
> 
> ssize_t?
> 
> +  r = r == -1 ? -errno : r;
> +  __set_errno (save_errno);
> +  return r;
> +}
>  
>  #define __poll_infinity_nocancel(fds, nfds) \
>    __poll (fds, nfds, -1)
> diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
> index a263d294b1..00ab75a405 100644
> --- a/sysdeps/unix/sysv/linux/not-cancel.h
> +++ b/sysdeps/unix/sysv/linux/not-cancel.h
> @@ -71,7 +71,7 @@ __writev_nocancel_nostatus (int fd, const struct iovec *iov, int iovcnt)
>  static inline int
> 
> ssize_t?
> 
>  __getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
>  {
> -  return INLINE_SYSCALL_CALL (getrandom, buf, buflen, flags);
> +  return INTERNAL_SYSCALL_CALL (getrandom, buf, buflen, flags);
>  }
> 
> Cheers,
> Wilco
  

On Thu, Sep 29, 2022 at 12:09 PM Adhemerval Zanella Netto via
Libc-alpha <libc-alpha@sourceware.org> wrote:
>
>
>
> On 29/09/22 15:36, Wilco Dijkstra wrote:
> > Hi Adhemerval,
> >
> > Another question, the syscall is defined as:
> >
> > ssize_t getrandom (void *__buffer, size_t __length,
> >
> > Doesn't this mean if we use 'int' for the return value, a large but valid syscall
> > result could be interpreted as a negative error value? It sounds like all code
> > processing the getrandom syscall should use ssize_t rather than int. Or do we
> > limit length to something fairly small?
>
> Yeah, you are right.  The syscall indeed returns ssize_t/long:
>
> include/linux/syscalls.h:1007:asmlinkage long sys_getrandom(char __user *buf, size_t count,
>
> So it does make sense to use ssize_t. It seems that not all architectures handle
> INTERNAL_SYSCALL consistently to use 'long', but at least the 64 bits does.

I think it should be __syscall_slong_t since sys_getrandom will return 64-bit
integer for x32.

> It also handles the issue raised by Yann, where arc4random fallback is not used.
> This is in fact another issue and I will send an independently patch.
>
> >
> >  __arc4random_buf (void *p, size_t n)
> >  {
> >    static int seen_initialized;
> > -  size_t l;
> > +  int l;
> >
> > Should be ssize_t?
> >
> > +static inline int
> > +__getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
> >
> > ssize_t?
> >
> > +{
> > +  int save_errno = errno;
> > +  int r = __getrandom (buf, buflen, flags);
> >
> > ssize_t?
> >
> > +  r = r == -1 ? -errno : r;
> > +  __set_errno (save_errno);
> > +  return r;
> > +}
> >
> >  #define __poll_infinity_nocancel(fds, nfds) \
> >    __poll (fds, nfds, -1)
> > diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
> > index a263d294b1..00ab75a405 100644
> > --- a/sysdeps/unix/sysv/linux/not-cancel.h
> > +++ b/sysdeps/unix/sysv/linux/not-cancel.h
> > @@ -71,7 +71,7 @@ __writev_nocancel_nostatus (int fd, const struct iovec *iov, int iovcnt)
> >  static inline int
> >
> > ssize_t?
> >
> >  __getrandom_nocancel (void *buf, size_t buflen, unsigned int flags)
> >  {
> > -  return INLINE_SYSCALL_CALL (getrandom, buf, buflen, flags);
> > +  return INTERNAL_SYSCALL_CALL (getrandom, buf, buflen, flags);
> >  }
> >
> > Cheers,
> > Wilco