The check for the extended signature table was backwards, so we always
ignored it.
We should verify that the extended signature table fits within the total
image size.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 73391 Build 70274: arc lint + arc unit
Event Timeline
| sys/x86/x86/ucode.c | ||
|---|---|---|
| 246 | Why this check is needed? Wouldn't the next check first addend give the same verification? BTW, how much do we want to check the blobs? eg are overflows believed to never occur? | |
| sys/x86/x86/ucode.c | ||
|---|---|---|
| 246 | I wanted to first check that it's "safe" to access table->signature_count. Overflow of the multiplication you mean? I believe it can't happen on 64-bit systems, but yes it could be written more carefully. In general I think we should at least make sure we're accessing fields within the bounds of the loaded microcode file. | |
| sys/x86/x86/ucode.c | ||
|---|---|---|
| 246 |
Please add one-line comment there.
I wonder if some kind of 'blob access API' which internally checks the bounds is due. It could be utilized in more places. But this is out of scope of this change, of course. | |
Comment Actions
Thanks both. Nice catch to fix the inverted condition. If I understand correctly, before this fix, we would never have detected an extended signature, and thus never applied microcode for processors only listed in the extended signature.
Comment Actions
That's right. I'm not sure how often that arises in practice. Certainly it's a severe bug. :(
| sys/x86/x86/ucode.c | ||
|---|---|---|
| 246 |
That would be useful. Something which lets you take a variety of "memory descriptors" (e.g., plain buffer, SG list of vaddrs or paddrs, mbufs, etc.) and maintains a current seek offset, then lets you copy in or copy out (more expensive, safer) or obtain a raw pointer to a sub-range of a specific size (cheaper, unsafe). The opencrypto code has something like this, see crypto_cursor_*, and I'm sure there are others. | |
Comment Actions
- Rework validition to avoid multiplication.
- Fall back to the extended signature table if the processor flags don't match.
Comment Actions
Accepting because everything looks correct to me, with the caveat that I don't often look at this code, and I haven't tested (because I don't have an Intel processor whose signature appears in an extended table).