[FFmpeg-devel] [PATCH] lavu/random_seed: use getrandom() when available

[FFmpeg-devel] [PATCH] lavu/random_seed: use getrandom() when available

[Anton Khirnov]

It is a better interface for /dev/u?random on Linux, which avoids the
issues associated with opening files.
---
 configure               |  2 ++
 libavutil/random_seed.c | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/configure b/configure
index d6e78297fe..a4b09577cf 100755
--- a/configure
+++ b/configure
@@ -2310,6 +2310,7 @@ SYSTEM_FUNCS="
     getauxval
     getenv
     gethrtime
+    getrandom
     getopt
     GetModuleHandle
     GetProcessAffinityMask
@@ -6387,6 +6388,7 @@ check_func  fcntl
 check_func  fork
 check_func  gethrtime
 check_func  getopt
+check_func_headers "sys/random.h" getrandom
 check_func  getrusage
 check_func  gettimeofday
 check_func  isatty
diff --git a/libavutil/random_seed.c b/libavutil/random_seed.c
index 2980e565e0..9a3a5aa133 100644
--- a/libavutil/random_seed.c
+++ b/libavutil/random_seed.c
@@ -35,6 +35,9 @@
 #elif CONFIG_OPENSSL
 #include <openssl/rand.h>
 #endif
+#if HAVE_GETRANDOM
+#include <sys/random.h>
+#endif
 #include <fcntl.h>
 #include <math.h>
 #include <time.h>
@@ -51,6 +54,7 @@
 #define TEST 0
 #endif
 
+#if !HAVE_GETRANDOM
 static int read_random(uint8_t *dst, size_t len, const char *file)
 {
 #if HAVE_UNISTD_H
@@ -70,6 +74,7 @@ static int read_random(uint8_t *dst, size_t len, const char *file)
     return AVERROR(ENOSYS);
 #endif
 }
+#endif
 
 static uint32_t get_generic_seed(void)
 {
@@ -147,7 +152,17 @@ int av_random_bytes(uint8_t* buf, size_t len)
     return 0;
 #endif
 
+    // getrandom() is a better interface for /dev/(u)random on Linux,
+    // so it makes no sense to try both
+#if HAVE_GETRANDOM
+    {
+        ssize_t read = getrandom(buf, len, GRND_NONBLOCK);
+        err = read < 0    ? AVERROR(errno)  :
+              read != len ? AVERROR_UNKNOWN : 0;
+    }
+#else
     err = read_random(buf, len, "/dev/urandom");
+#endif
     if (!err)
         return err;
 
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/random_seed: use getrandom() when available

[James Almer]

On 7/7/2023 7:21 AM, Anton Khirnov wrote:
> It is a better interface for /dev/u?random on Linux, which avoids the
> issues associated with opening files.
> ---
>   configure               |  2 ++
>   libavutil/random_seed.c | 15 +++++++++++++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/configure b/configure
> index d6e78297fe..a4b09577cf 100755
> --- a/configure
> +++ b/configure
> @@ -2310,6 +2310,7 @@ SYSTEM_FUNCS="
>       getauxval
>       getenv
>       gethrtime
> +    getrandom
>       getopt
>       GetModuleHandle
>       GetProcessAffinityMask
> @@ -6387,6 +6388,7 @@ check_func  fcntl
>   check_func  fork
>   check_func  gethrtime
>   check_func  getopt
> +check_func_headers "sys/random.h" getrandom
>   check_func  getrusage
>   check_func  gettimeofday
>   check_func  isatty
> diff --git a/libavutil/random_seed.c b/libavutil/random_seed.c
> index 2980e565e0..9a3a5aa133 100644
> --- a/libavutil/random_seed.c
> +++ b/libavutil/random_seed.c
> @@ -35,6 +35,9 @@
>   #elif CONFIG_OPENSSL
>   #include <openssl/rand.h>
>   #endif
> +#if HAVE_GETRANDOM
> +#include <sys/random.h>
> +#endif
>   #include <fcntl.h>
>   #include <math.h>
>   #include <time.h>
> @@ -51,6 +54,7 @@
>   #define TEST 0
>   #endif
>   
> +#if !HAVE_GETRANDOM
>   static int read_random(uint8_t *dst, size_t len, const char *file)
>   {
>   #if HAVE_UNISTD_H
> @@ -70,6 +74,7 @@ static int read_random(uint8_t *dst, size_t len, const char *file)
>       return AVERROR(ENOSYS);
>   #endif
>   }
> +#endif
>   
>   static uint32_t get_generic_seed(void)
>   {
> @@ -147,7 +152,17 @@ int av_random_bytes(uint8_t* buf, size_t len)
>       return 0;
>   #endif
>   
> +    // getrandom() is a better interface for /dev/(u)random on Linux,
> +    // so it makes no sense to try both
> +#if HAVE_GETRANDOM
> +    {
> +        ssize_t read = getrandom(buf, len, GRND_NONBLOCK);
> +        err = read < 0    ? AVERROR(errno)  :
> +              read != len ? AVERROR_UNKNOWN : 0;

The documentation states

"By default, when reading from the random source, getrandom() blocks if 
no random bytes are available, and when reading from the urandom source, 
it blocks if the entropy pool has not yet been initialized.If the 
GRND_NONBLOCK flag is set, then getrandom() does not block in these 
cases, but instead immediately returns -1 with errno set to EAGAIN."

Returning EAGAIN may end up clashing with our usage of said error value 
internally (Marton's patch will make use of this function in hls), so 
maybe prevent said value from propagating here.

LGTM otherwise.

> +    }
> +#else
>       err = read_random(buf, len, "/dev/urandom");
> +#endif
>       if (!err)
>           return err;
>   
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/random_seed: use getrandom() when available

[Marton Balint]

On Fri, 7 Jul 2023, Anton Khirnov wrote:

> It is a better interface for /dev/u?random on Linux, which avoids the
> issues associated with opening files.


getrandom() actually have the same problem as read(). It can read less 
than requested. So you should use it in a loop in that case or if it 
returns EINTR.

Regards,
Marton


> ---
> configure               |  2 ++
> libavutil/random_seed.c | 15 +++++++++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/configure b/configure
> index d6e78297fe..a4b09577cf 100755
> --- a/configure
> +++ b/configure
> @@ -2310,6 +2310,7 @@ SYSTEM_FUNCS="
>     getauxval
>     getenv
>     gethrtime
> +    getrandom
>     getopt
>     GetModuleHandle
>     GetProcessAffinityMask
> @@ -6387,6 +6388,7 @@ check_func  fcntl
> check_func  fork
> check_func  gethrtime
> check_func  getopt
> +check_func_headers "sys/random.h" getrandom
> check_func  getrusage
> check_func  gettimeofday
> check_func  isatty
> diff --git a/libavutil/random_seed.c b/libavutil/random_seed.c
> index 2980e565e0..9a3a5aa133 100644
> --- a/libavutil/random_seed.c
> +++ b/libavutil/random_seed.c
> @@ -35,6 +35,9 @@
> #elif CONFIG_OPENSSL
> #include <openssl/rand.h>
> #endif
> +#if HAVE_GETRANDOM
> +#include <sys/random.h>
> +#endif
> #include <fcntl.h>
> #include <math.h>
> #include <time.h>
> @@ -51,6 +54,7 @@
> #define TEST 0
> #endif
>
> +#if !HAVE_GETRANDOM
> static int read_random(uint8_t *dst, size_t len, const char *file)
> {
> #if HAVE_UNISTD_H
> @@ -70,6 +74,7 @@ static int read_random(uint8_t *dst, size_t len, const char *file)
>     return AVERROR(ENOSYS);
> #endif
> }
> +#endif
>
> static uint32_t get_generic_seed(void)
> {
> @@ -147,7 +152,17 @@ int av_random_bytes(uint8_t* buf, size_t len)
>     return 0;
> #endif
>
> +    // getrandom() is a better interface for /dev/(u)random on Linux,
> +    // so it makes no sense to try both
> +#if HAVE_GETRANDOM
> +    {
> +        ssize_t read = getrandom(buf, len, GRND_NONBLOCK);
> +        err = read < 0    ? AVERROR(errno)  :
> +              read != len ? AVERROR_UNKNOWN : 0;
> +    }
> +#else
>     err = read_random(buf, len, "/dev/urandom");
> +#endif
>     if (!err)
>         return err;
>
> -- 
> 2.40.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/random_seed: use getrandom() when available

[Anton Khirnov]

Quoting Marton Balint (2023-07-07 22:02:26)
> 
> 
> On Fri, 7 Jul 2023, Anton Khirnov wrote:
> 
> > It is a better interface for /dev/u?random on Linux, which avoids the
> > issues associated with opening files.
> 
> 
> getrandom() actually have the same problem as read(). It can read less 
> than requested. So you should use it in a loop in that case or if it 
> returns EINTR.

I'm not convinced it's actually a problem.

This API is intended for small secrets like keys and such, somebody
trying to generate vast quantities of random data is likely misusing it
and could just as well use LFG or something.
Failing in that case seems like a good thing to me.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] lavu/random_seed: use getrandom() when available

[Anton Khirnov]

It is a better interface for /dev/u?random on Linux, which avoids the
issues associated with opening files.
---
 configure               |  2 ++
 libavutil/random_seed.c | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/configure b/configure
index 0ab0761011..f8c809e491 100755
--- a/configure
+++ b/configure
@@ -2310,6 +2310,7 @@ SYSTEM_FUNCS="
     getauxval
     getenv
     gethrtime
+    getrandom
     getopt
     GetModuleHandle
     GetProcessAffinityMask
@@ -6387,6 +6388,7 @@ check_func  fcntl
 check_func  fork
 check_func  gethrtime
 check_func  getopt
+check_func_headers "sys/random.h" getrandom
 check_func  getrusage
 check_func  gettimeofday
 check_func  isatty
diff --git a/libavutil/random_seed.c b/libavutil/random_seed.c
index f167b172f2..92bf9ef8e1 100644
--- a/libavutil/random_seed.c
+++ b/libavutil/random_seed.c
@@ -35,6 +35,9 @@
 #elif CONFIG_OPENSSL
 #include <openssl/rand.h>
 #endif
+#if HAVE_GETRANDOM
+#include <sys/random.h>
+#endif
 #include <fcntl.h>
 #include <math.h>
 #include <time.h>
@@ -51,6 +54,7 @@
 #define TEST 0
 #endif
 
+#if !HAVE_GETRANDOM
 static int read_random(uint8_t *dst, size_t len, const char *file)
 {
 #if HAVE_UNISTD_H
@@ -70,6 +74,7 @@ static int read_random(uint8_t *dst, size_t len, const char *file)
     return AVERROR(ENOSYS);
 #endif
 }
+#endif
 
 static uint32_t get_generic_seed(void)
 {
@@ -147,7 +152,18 @@ int av_random_bytes(uint8_t* buf, size_t len)
     return 0;
 #endif
 
+    // getrandom() is a better interface for /dev/(u)random on Linux,
+    // so it makes no sense to try both
+#if HAVE_GETRANDOM
+    {
+        ssize_t read = getrandom(buf, len, GRND_NONBLOCK);
+        if (read < 0)
+            return errno == EAGAIN ? AVERROR_EXTERNAL : AVERROR(errno);
+        return read != len ? AVERROR_UNKNOWN : 0;
+    }
+#else
     err = read_random(buf, len, "/dev/urandom");
+#endif
     if (!err)
         return err;
 
-- 
2.40.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/random_seed: use getrandom() when available

[Marton Balint]

On Sun, 9 Jul 2023, Anton Khirnov wrote:

> Quoting Marton Balint (2023-07-07 22:02:26)
>>
>>
>> On Fri, 7 Jul 2023, Anton Khirnov wrote:
>>
>>> It is a better interface for /dev/u?random on Linux, which avoids the
>>> issues associated with opening files.
>>
>>
>> getrandom() actually have the same problem as read(). It can read less
>> than requested. So you should use it in a loop in that case or if it
>> returns EINTR.
>
> I'm not convinced it's actually a problem.
>
> This API is intended for small secrets like keys and such, somebody
> trying to generate vast quantities of random data is likely misusing it
> and could just as well use LFG or something.
>
> Failing in that case seems like a good thing to me.

This is a very bad argument. If the API should not be used for big 
secrets, then it should always fail for size > 256 or something, not 
sometimes fail. And such limitation should be documented.

And its not just about big secrets. EINTR can be returned for small 
secrets as well, and you should handle it.

I also question if it is a good idea to use the non blocking mode. Imagine 
a situation when somebody wants to automatically start a command line 
after boot which needs a key. It might fail right after boot, but will 
work after a couple of minutes. IMHO it is better to block (1-2 minute 
tops) than making the function sometimes work, sometimes not.

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/random_seed: use getrandom() when available

[James Almer]

On 7/9/2023 1:23 PM, Marton Balint wrote:
> 
> 
> On Sun, 9 Jul 2023, Anton Khirnov wrote:
> 
>> Quoting Marton Balint (2023-07-07 22:02:26)
>>>
>>>
>>> On Fri, 7 Jul 2023, Anton Khirnov wrote:
>>>
>>>> It is a better interface for /dev/u?random on Linux, which avoids the
>>>> issues associated with opening files.
>>>
>>>
>>> getrandom() actually have the same problem as read(). It can read less
>>> than requested. So you should use it in a loop in that case or if it
>>> returns EINTR.
>>
>> I'm not convinced it's actually a problem.
>>
>> This API is intended for small secrets like keys and such, somebody
>> trying to generate vast quantities of random data is likely misusing it
>> and could just as well use LFG or something.
>>
>> Failing in that case seems like a good thing to me.
> 
> This is a very bad argument. If the API should not be used for big 
> secrets, then it should always fail for size > 256 or something, not 
> sometimes fail. And such limitation should be documented.
> 
> And its not just about big secrets. EINTR can be returned for small 
> secrets as well, and you should handle it.
> 
> I also question if it is a good idea to use the non blocking mode. 
> Imagine a situation when somebody wants to automatically start a command 
> line after boot which needs a key. It might fail right after boot, but 
> will work after a couple of minutes. IMHO it is better to block (1-2 
> minute tops) than making the function sometimes work, sometimes not.

If we make the urandom/getrandom() path block and potentially take 1-2 
minutes, then I'd prefer if it's last in the function, after all 
available external implementations (Some of which can fail) were tried 
first.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/random_seed: use getrandom() when available

[Marton Balint]

On Sun, 9 Jul 2023, James Almer wrote:

> On 7/9/2023 1:23 PM, Marton Balint wrote:
>>
>>
>>  On Sun, 9 Jul 2023, Anton Khirnov wrote:
>>
>>>  Quoting Marton Balint (2023-07-07 22:02:26)
>>>> 
>>>>
>>>>  On Fri, 7 Jul 2023, Anton Khirnov wrote:
>>>>
>>>>>  It is a better interface for /dev/u?random on Linux, which avoids the
>>>>>  issues associated with opening files.
>>>> 
>>>>
>>>>  getrandom() actually have the same problem as read(). It can read less
>>>>  than requested. So you should use it in a loop in that case or if it
>>>>  returns EINTR.
>>>
>>>  I'm not convinced it's actually a problem.
>>>
>>>  This API is intended for small secrets like keys and such, somebody
>>>  trying to generate vast quantities of random data is likely misusing it
>>>  and could just as well use LFG or something.
>>>
>>>  Failing in that case seems like a good thing to me.
>>
>>  This is a very bad argument. If the API should not be used for big
>>  secrets, then it should always fail for size > 256 or something, not
>>  sometimes fail. And such limitation should be documented.
>>
>>  And its not just about big secrets. EINTR can be returned for small
>>  secrets as well, and you should handle it.
>>
>>  I also question if it is a good idea to use the non blocking mode. Imagine
>>  a situation when somebody wants to automatically start a command line
>>  after boot which needs a key. It might fail right after boot, but will
>>  work after a couple of minutes. IMHO it is better to block (1-2 minute
>>  tops) than making the function sometimes work, sometimes not.
>
> If we make the urandom/getrandom() path block and potentially take 1-2 
> minutes, then I'd prefer if it's last in the function, after all available 
> external implementations (Some of which can fail) were tried first.

It will take 1-2 minutes if it is right after boot and if the system has 
no support for quickly gathering entropy via some hardware feature...

So it is not like /dev/random. It only waits until the CSPRNG of 
/dev/urandom initializes. I doubt external libraries can come up something 
significantly quicker, since they are also using the same system provided 
randomness for initialization...

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavu/random_seed: use getrandom() when available

[James Almer]

On 7/9/2023 7:06 AM, Anton Khirnov wrote:
> It is a better interface for /dev/u?random on Linux, which avoids the
> issues associated with opening files.
> ---
>   configure               |  2 ++
>   libavutil/random_seed.c | 16 ++++++++++++++++
>   2 files changed, 18 insertions(+)
> 
> diff --git a/configure b/configure
> index 0ab0761011..f8c809e491 100755
> --- a/configure
> +++ b/configure
> @@ -2310,6 +2310,7 @@ SYSTEM_FUNCS="
>       getauxval
>       getenv
>       gethrtime
> +    getrandom
>       getopt
>       GetModuleHandle
>       GetProcessAffinityMask
> @@ -6387,6 +6388,7 @@ check_func  fcntl
>   check_func  fork
>   check_func  gethrtime
>   check_func  getopt
> +check_func_headers "sys/random.h" getrandom
>   check_func  getrusage
>   check_func  gettimeofday
>   check_func  isatty
> diff --git a/libavutil/random_seed.c b/libavutil/random_seed.c
> index f167b172f2..92bf9ef8e1 100644
> --- a/libavutil/random_seed.c
> +++ b/libavutil/random_seed.c
> @@ -35,6 +35,9 @@
>   #elif CONFIG_OPENSSL
>   #include <openssl/rand.h>
>   #endif
> +#if HAVE_GETRANDOM
> +#include <sys/random.h>
> +#endif
>   #include <fcntl.h>
>   #include <math.h>
>   #include <time.h>
> @@ -51,6 +54,7 @@
>   #define TEST 0
>   #endif
>   
> +#if !HAVE_GETRANDOM
>   static int read_random(uint8_t *dst, size_t len, const char *file)
>   {
>   #if HAVE_UNISTD_H
> @@ -70,6 +74,7 @@ static int read_random(uint8_t *dst, size_t len, const char *file)
>       return AVERROR(ENOSYS);
>   #endif
>   }
> +#endif
>   
>   static uint32_t get_generic_seed(void)
>   {
> @@ -147,7 +152,18 @@ int av_random_bytes(uint8_t* buf, size_t len)
>       return 0;
>   #endif
>   
> +    // getrandom() is a better interface for /dev/(u)random on Linux,
> +    // so it makes no sense to try both
> +#if HAVE_GETRANDOM
> +    {
> +        ssize_t read = getrandom(buf, len, GRND_NONBLOCK);
> +        if (read < 0)
> +            return errno == EAGAIN ? AVERROR_EXTERNAL : AVERROR(errno);
> +        return read != len ? AVERROR_UNKNOWN : 0;

This is returning no matter what the result, when it should only return 
on success (the !err check below), or fallback to the other 
implementations otherwise.

> +    }
> +#else
>       err = read_random(buf, len, "/dev/urandom");
> +#endif
>       if (!err)
>           return err;
>   
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".