Fix IAR warning about volatile access#10045
Conversation
There was a problem hiding this comment.
Pull request overview
Updates RSA unpadding and ticket decryption error masking to satisfy IAR’s stricter rules around volatile access by first reading volatile values into local copies and using those in subsequent expressions.
Changes:
- Refactored constant-time RSA unpadding checks to use local copies of volatile variables in expressions.
- Updated ticket encryption callback RSA path to use a local copy of a volatile mask when composing
lastErr.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| wolfcrypt/src/rsa.c | Reads volatile masking state into local temporaries before combining/deriving masks to avoid IAR volatile-use warnings. |
| src/internal.c | Uses a local copy of lenErrMask when building args->lastErr to avoid volatile access in compound expressions. |
Comments suppressed due to low confidence (2)
src/internal.c:1
- Since
lenErrMaskCopyis the value actually used in subsequent expressions, computing directly into the non-volatile local (and only keepingvolatileif it’s required for the toolchain workaround) would reduce noise and avoid an extra volatile write+read pair. For example, compute the mask intolenErrMaskCopyand use that consistently.
src/internal.c:1 - Since
lenErrMaskCopyis the value actually used in subsequent expressions, computing directly into the non-volatile local (and only keepingvolatileif it’s required for the toolchain workaround) would reduce noise and avoid an extra volatile write+read pair. For example, compute the mask intolenErrMaskCopyand use that consistently.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| volatile byte invalid = 0; | ||
| volatile byte minPad; | ||
| volatile int invalidMask; | ||
| word16 pastSepCopy; |
There was a problem hiding this comment.
pastSepCopy is declared as word16 but is later complemented and truncated to byte. If pastSep is intended to be an 8-bit mask (as implied by (byte)~pastSep previously), widening to 16-bit before ~ can change the intermediate value and makes the operation dependent on implicit promotions/truncation. Prefer declaring pastSepCopy with the same type/width as pastSep (or explicitly cast before the bitwise NOT) to preserve exact semantics.
| pastSepCopy = pastSep; | ||
| invalidCopy = invalid; | ||
| invalid = invalidCopy | (byte)~pastSepCopy; |
There was a problem hiding this comment.
pastSepCopy is declared as word16 but is later complemented and truncated to byte. If pastSep is intended to be an 8-bit mask (as implied by (byte)~pastSep previously), widening to 16-bit before ~ can change the intermediate value and makes the operation dependent on implicit promotions/truncation. Prefer declaring pastSepCopy with the same type/width as pastSep (or explicitly cast before the bitwise NOT) to preserve exact semantics.
| minPadCopy = minPad; | ||
| invalidCopy = invalid; | ||
| invalid = invalidCopy | minPadCopy; | ||
| /* Must have seen separator. */ | ||
| invalid |= (byte)~pastSep; | ||
| pastSepCopy = pastSep; | ||
| invalidCopy = invalid; | ||
| invalid = invalidCopy | (byte)~pastSepCopy; | ||
| /* First byte must be 0x00. */ | ||
| invalid |= ctMaskNotEq(pkcsBlock[0], 0x00); | ||
| invalidCopy = invalid; | ||
| invalid = invalidCopy | ctMaskNotEq(pkcsBlock[0], 0x00); | ||
| /* Check against expected block type: padValue */ | ||
| invalid |= ctMaskNotEq(pkcsBlock[1], padValue); | ||
| invalidCopy = invalid; | ||
| invalid = invalidCopy | ctMaskNotEq(pkcsBlock[1], padValue); |
There was a problem hiding this comment.
This repeated volatile-to-local copy pattern is easy to get wrong over time and makes the constant-time logic harder to audit. Consider consolidating this into a small helper/macro (e.g., a READ_VOLATILE(x) and/or OR_ASSIGN_VOLATILE(dst, src) pattern) so each operation is expressed once and the IAR workaround is localized.
Description
Read the volatile once into a local copy, then use the copy in expressions — satisfying IAR's stricter volatile access rules.
Fixes zd21385
Testing
Customer confirmed clean build with IAR tools
Checklist