Re: [PATCHv2 2/2] hwrng: optee: fix wait use case

On 05/08/20, Sumit Garg wrote:

Apologies for my delayed response as I was busy with some other tasks
along with holidays.

no pb! was just making sure this wasnt falling through some cracks.

On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries
[off-list ref] wrote:

quoted

On 24/07/20, Sumit Garg wrote:

quoted

On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz [off-list ref] wrote:

quoted

The current code waits for data to be available before attempting a
second read. However the second read would not be executed as the
while loop exits.

This fix does not wait if all data has been read and reads a second
time if only partial data was retrieved on the first read.

This fix also does not attempt to read if not data is requested.

I am not sure how this is possible, can you elaborate?

currently, if the user sets max 0, get_optee_rng_data will regardless
issuese a call to the secure world requesting 0 bytes from the RNG

This case is already handled by core API: rng_dev_read().

ah ok good point, you are right
but yeah, there is no consequence to the actual patch.

quoted

with this patch, this request is avoided.

quoted

quoted

Signed-off-by: Jorge Ramirez-Ortiz <redacted>
---
 v2: tidy up the while loop to avoid reading when no data is requested

 drivers/char/hw_random/optee-rng.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c
index 5bc4700c4dae..a99d82949981 100644
--- a/drivers/char/hw_random/optee-rng.c
+++ b/drivers/char/hw_random/optee-rng.c

@@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
        if (max > MAX_ENTROPY_REQ_SZ)
                max = MAX_ENTROPY_REQ_SZ;

-       while (read == 0) {
+       while (read < max) {
                rng_size = get_optee_rng_data(pvt_data, data, (max - read));

                data += rng_size;
                read += rng_size;

                if (wait && pvt_data->data_rate) {
-                       if (timeout-- == 0)
+                       if ((timeout-- == 0) || (read == max))

If read == max, would there be any sleep?

no but I see no reason why there should be a wait since we already have
all the data that we need; the msleep is only required when we need to
wait for the RNG to generate entropy for the number of bytes we are
requesting. if we are requesting 0 bytes, the entropy is already
available. at leat this is what makes sense to me.

Wouldn't it lead to a call as msleep(0); that means no wait as well?

I dont understand: there is no reason to wait if read == max and this
patch will not wait: if read == max it calls 'return read'

am I misunderstanding your point?

-Sumit

quoted

quoted

-Sumit

quoted

                                return read;
                        msleep((1000 * (max - read)) / pvt_data->data_rate);
                } else {
--
2.17.1