public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2] malloc: Do not clobber errno on __getrandom_nocancel (BZ#29624)
@ 2022-09-29 17:55 Adhemerval Zanella
  2022-09-29 18:36 ` Wilco Dijkstra
  2022-09-29 18:44 ` Yann Droneaud
  0 siblings, 2 replies; 5+ messages in thread
From: Adhemerval Zanella @ 2022-09-29 17:55 UTC (permalink / raw)
  To: libc-alpha, Wilco Dijkstra, Yu Chien Peter Lin

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)
+      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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] malloc: Do not clobber errno on __getrandom_nocancel (BZ#29624)
  2022-09-29 17:55 [PATCH v2] malloc: Do not clobber errno on __getrandom_nocancel (BZ#29624) Adhemerval Zanella
@ 2022-09-29 18:36 ` Wilco Dijkstra
  2022-09-29 19:07   ` Adhemerval Zanella Netto
  2022-09-29 18:44 ` Yann Droneaud
  1 sibling, 1 reply; 5+ messages in thread
From: Wilco Dijkstra @ 2022-09-29 18:36 UTC (permalink / raw)
  To: Adhemerval Zanella, libc-alpha, Yu Chien Peter Lin

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] malloc: Do not clobber errno on __getrandom_nocancel (BZ#29624)
  2022-09-29 17:55 [PATCH v2] malloc: Do not clobber errno on __getrandom_nocancel (BZ#29624) Adhemerval Zanella
  2022-09-29 18:36 ` Wilco Dijkstra
@ 2022-09-29 18:44 ` Yann Droneaud
  1 sibling, 0 replies; 5+ messages in thread
From: Yann Droneaud @ 2022-09-29 18:44 UTC (permalink / raw)
  To: Adhemerval Zanella, libc-alpha

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>

-- 
Yann Droneaud
OPTEYA

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] malloc: Do not clobber errno on __getrandom_nocancel (BZ#29624)
  2022-09-29 18:36 ` Wilco Dijkstra
@ 2022-09-29 19:07   ` Adhemerval Zanella Netto
  2022-09-29 19:36     ` H.J. Lu
  0 siblings, 1 reply; 5+ messages in thread
From: Adhemerval Zanella Netto @ 2022-09-29 19:07 UTC (permalink / raw)
  To: Wilco Dijkstra, libc-alpha, Yu Chien Peter Lin, Yann Droneaud



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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] malloc: Do not clobber errno on __getrandom_nocancel (BZ#29624)
  2022-09-29 19:07   ` Adhemerval Zanella Netto
@ 2022-09-29 19:36     ` H.J. Lu
  0 siblings, 0 replies; 5+ messages in thread
From: H.J. Lu @ 2022-09-29 19:36 UTC (permalink / raw)
  To: Adhemerval Zanella Netto
  Cc: Wilco Dijkstra, libc-alpha, Yu Chien Peter Lin, Yann Droneaud

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



-- 
H.J.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-09-29 19:37 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-29 17:55 [PATCH v2] malloc: Do not clobber errno on __getrandom_nocancel (BZ#29624) Adhemerval Zanella
2022-09-29 18:36 ` Wilco Dijkstra
2022-09-29 19:07   ` Adhemerval Zanella Netto
2022-09-29 19:36     ` H.J. Lu
2022-09-29 18:44 ` Yann Droneaud

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).