Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libc: Refine the arc4random_buf implementation #14509

Merged
merged 1 commit into from
Oct 26, 2024

Conversation

xiaoxiang781216
Copy link
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

fill the buffer with getrandom instead random pool and move the implementation to from crypto to libc

Impact

Make arc4random_buf always available

Testing

ci

@github-actions github-actions bot added Area: Networking Effects networking subsystem Area: OS Components OS Components issues Area: Crypto Size: M The size of the change in this PR is medium labels Oct 25, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 25, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements.

Missing Information:

  • Summary:
    • Lacks a clear explanation of why the change from random pool to getrandom is necessary. What problem does it solve?
    • Needs more detail on the functional code changes. "move the implementation from crypto to libc" is vague. Be specific about functions or modules impacted.
  • Impact:
    • While "Make arc4random_buf always available" is a result, explain how this impacts users or the system. Is there a performance difference? Why is this availability important?
    • The impact sections are largely incomplete. Address all points (build, hardware, documentation, security, compatibility) with either "NO" or a "YES" + explanation.
  • Testing:
    • "ci" is insufficient. Provide actual build host and target details as requested.
    • "Testing logs before/after" are missing entirely. Show evidence of the problem the PR fixes AND that it now works as intended.

Recommendations:

  1. Expand the Summary: Clearly articulate the problem and solution. Provide specific code-level details.
  2. Complete Impact Assessment: Address all impact points. Even if the answer is "NO", state it explicitly.
  3. Provide Detailed Testing Information: Include build host/target specifics and relevant logs demonstrating the change's effect.

By providing this missing information, your PR will be much stronger and easier for reviewers to understand and approve.

xiaoxiang781216 added a commit to xiaoxiang781216/incubator-nuttx-apps that referenced this pull request Oct 25, 2024
and follow the kernel side change:
apache/nuttx#14509

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
xiaoxiang781216 added a commit to xiaoxiang781216/incubator-nuttx-apps that referenced this pull request Oct 25, 2024
and follow the kernel side change:
apache/nuttx#14509

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
@acassis
Copy link
Contributor

acassis commented Oct 25, 2024

@xiaoxiang781216 maybe we need to update the random number generation Documentation/ to explain that up_rngbuf() now is used to fill the randomness pool

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @xiaoxiang781216 :-) Build errors :-) + documentation update needed? :-)

fill the buffer with getrandom instead random pool
and move the implementation to from crypto to libc

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
@xiaoxiang781216
Copy link
Contributor Author

@xiaoxiang781216 maybe we need to update the random number generation Documentation/ to explain that up_rngbuf() now is used to fill the randomness pool

but we don't have any document which describe random pool.

@xiaoxiang781216
Copy link
Contributor Author

Thank you @xiaoxiang781216 :-) Build errors :-) + documentation update needed? :-)

fixed.

@cederom
Copy link
Contributor

cederom commented Oct 25, 2024

@acassis: @xiaoxiang781216 maybe we need to update the random number generation Documentation/ to explain that up_rngbuf() now is used to fill the randomness pool

@xiaoxiang781216: but we don't have any document which describe random pool.

Okay then nothing to update :-P But I agree some documentation would be nice to have here (different task), random is important, for instance would random work without hardware entropy generator and with software implementation with arc4random and how that impacts security (i.e. mbedtls)? :-)

@acassis
Copy link
Contributor

acassis commented Oct 25, 2024

@xiaoxiang781216 @cederom exactly! How to integrate HW RNG, how to use pseudo random generator without HW RNG, etc

@cederom
Copy link
Contributor

cederom commented Oct 25, 2024

Okay, TODO added in Issue #14511 :-)

@acassis acassis merged commit 32784b0 into apache:master Oct 26, 2024
27 checks passed
@xiaoxiang781216 xiaoxiang781216 deleted the random branch October 27, 2024 02:50
GUIDINGLI pushed a commit to apache/nuttx-apps that referenced this pull request Oct 27, 2024
and follow the kernel side change:
apache/nuttx#14509

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
@lupyuen
Copy link
Member

lupyuen commented Oct 27, 2024

Sorry @xiaoxiang781216: The arm-04 build is failing at the NuttX Mirror Repo, wonder if it's because of this PR? Thanks!

Configuration/Tool: lc823450-xgevk/krndis,CONFIG_ARM_TOOLCHAIN_GNU_EABI
arm-none-eabi-ld: /github/workspace/sources/nuttx/staging//libc.a(lib_arc4random.o): in function `arc4random_buf':
/github/workspace/sources/nuttx/libs/libc/stdlib/lib_arc4random.c:111:(.text.arc4random_buf+0x26): undefined reference to `clock_systime_ticks'

https://github.com/NuttX/nuttx/actions/runs/11540264972/job/32129220872#step:7:101

Update: This error is strange. Sometimes it doesn't appear: https://github.com/NuttX/nuttx/actions/runs/11540270335

But it appears on my Home Build Server: https://gist.github.com/nuttxpr/74e46f5eca2a0cd5a234e5389d40457a#file-ci-arm-04-log-L157

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Oct 28, 2024

@lupyuen Fix here: #14523

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Crypto Area: Networking Effects networking subsystem Area: OS Components OS Components issues Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants