VMS: modernise rand_pool_acquire_entropy#6151
VMS: modernise rand_pool_acquire_entropy#6151levitte wants to merge 15 commits intoopenssl:masterfrom
Conversation
levitte
added
branch: master
|
Note: together with #6150, the entropy factor will be made larger (i.e. the VMS entropy gatherer will become much more pessimistic about the quality of the entropy sources) |
Stop redefining structures that are already defined in system headers. This also means we can stop setting the pointer size globally, because the system structures will have the correct pointer sizes either way. The only exception is passing the right pointer size to a function. Stop trying to twist things around with rand(), that's the job of the DRBG that we feed. Stop assuming the location of the JPI$_FINALEXC item, look it up instead. Signal an exception if the sys$getjpiw call fails (it means the item list isn't set up right, so works as an assertion, but using VMS methodology).
Add more items that could serve as entropy source.
levitte
force-pushed
the
VMS-modernise-rand-gathering
branch
from
e8df477 to
c510292
Compare
|
Rebased and entropy factor updated. |
crypto/rand/rand_vms.c
Outdated
| * Input: | ||
| * items_data - an array of lengths and codes | ||
| * items_data_num - number of elements in that array, minus one | ||
| * (caller MUST have space for one extra NULL element) |
There was a problem hiding this comment.
"minus one"? What am I missing? It looks like it's just the "number of elements in that array". Caller indeed must have space for one extra NULL element, but in the output array, not that.
There was a problem hiding this comment.
Thanks, that's a misplaced comment. Fixed
crypto/rand/rand_vms.c
Outdated
| else | ||
| pitems->ile3$w_length = pitems_input->length; | ||
|
|
||
| pitems->ile3$w_code = pitems_input->code; |
There was a problem hiding this comment.
Formatting nit, too many spaces. [Or too little if intention was to align equal signs in multiple lines.]
There was a problem hiding this comment.
It was previously aligned with the assignment of ->ile3$w_length. Fixed.
|
I currently get a mysterious crash on IA64... need to resolve that before approval |
|
Some of output sizes are 8. Does it imply corresponding alignment? If so, it might be the reason. I mean if it's supposed to be aligned, then there are some that won't be. Because number of prior 4-byte sized elements is not even. EDIT: If alignment turns to be the issue, it would be only appropriate to ensure data_buff's alignment. Compiler is likely to align it at 8-bytes, 64-bit compilers customarily do, but it doesn't have to... |
|
Ooooh, I think you're on to something. The data_buffer pointer you're thinking about points at an array of 32-bit integers, and if the 8-byte element happen to be an odd element, it will be positioned across a 64-bit boundary. I think that the easiest is to make the array a |
|
Or hmmm... |
|
That did change something... now it crashes elsewhere... |
crypto/rand/rand_vms.c
Outdated
| {4, JPI$_DIRIO}, | ||
| {4, JPI$_IMAGECOUNT}, | ||
| {8, JPI$_LAST_LOGIN_I}, | ||
| {8, JPI$_LOGINTIM}, |
There was a problem hiding this comment.
I think you meant to remove this these two...
crypto/rand/rand_vms.c
Outdated
| ILE3 RMI_items[OSSL_NELEM(RMI_item_data) | ||
| + OSSL_NELEM(RMI_item_data_64bit) + 1]; | ||
| ILE3 SYI_items[OSSL_NELEM(SYI_item_data) + 1]; | ||
| uint32_t data_buffer[OSSL_NELEM(JPI_item_data_64bit) * 2 |
There was a problem hiding this comment.
If alignment is significant you still need to ensure alignment of data_buffer. For example union { double align; uint32_t data[OSSL_NELEM... ]; } buffer;, and then passing &buffer.data[total_length]...
| != SS$_NORMAL) { | ||
| lib$signal(status); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
But doesn't split mean that prepare_item_list would [have to] grow JPI_items, not overwrite 64-bit queries with 32-bit ones? Or you'd need pair of getjpiw calls...
There was a problem hiding this comment.
Yup, I realised that just now. Fix coming up
crypto/rand/rand_vms.c
Outdated
| total_length_64bit += | ||
| prepare_item_list(RMI_item_data_64bit, OSSL_NELEM(RMI_item_data_64bit), | ||
| RMI_items_64bit, | ||
| &data_buffer[total_length]); |
There was a problem hiding this comment.
&data_buffer[total_length_64bit] not &data_buffer[total_length]
There was a problem hiding this comment.
Darnit!!!
...
I mean, good catch! Thanks. Fixed
crypto/rand/rand_vms.c
Outdated
| uint32_t__ptr32 databuffer) | ||
| { | ||
| const struct item_st *pitems_input; | ||
| ILE3 *pitems; |
There was a problem hiding this comment.
On side note, formally you don't need to reassign items_input or items, you can use arguments directly.
crypto/rand/rand_vms.c
Outdated
| &data_buffer_64bit[total_length_64bit]); | ||
| /* Now the 32-bit items */ | ||
| total_length += prepare_item_list(DVI_item_data, OSSL_NELEM(DVI_item_data), | ||
| DVI_items, &data_buffer[total_length]); |
There was a problem hiding this comment.
Wait a sec! total_length is in bytes, so where would &data_buffer[total_length] point? There ought to be your crash. I mean maybe it wasn't about alignment after all...
There was a problem hiding this comment.
maybe it wasn't about alignment after all...
But if it is, then data_buffer_64bit still needs alignment insurance.
There was a problem hiding this comment.
It seems that insurance is still needed
|
Away a few hours, I'll get back to this tonight |
crypto/rand/rand_vms.c
Outdated
| pitems->ile3$ps_bufaddr = databuffer; | ||
| pitems->ile3$ps_retlen_addr = 0; | ||
|
|
||
| databuffer += pitems_input->length / sizeof(*databuffer); |
There was a problem hiding this comment.
For consistency change to sizeof(databuffer[0]).
| rand_pool_add(pool, (PTR_T)data_buffer, total_length, | ||
| total_length / ENTROPY_FACTOR); | ||
| rand_pool_add(pool, (unsigned char *)data_buffer, total_length, | ||
| 8 * total_length / ENTROPY_FACTOR); |
There was a problem hiding this comment.
Is compiler allowed to optimize this as (8 / ENTROPY_FACTOR) * total_length? Which would yield 0 for any total_length... In other words I think explicit (8 * total_length) / ENTROPY_FACTOR would be appropriate here...
There was a problem hiding this comment.
I thought * and / were left-to-right operators. Are compilers allowed to move around operations like you suggest, even though that would give different results?
There was a problem hiding this comment.
The language spec mandates an order of operations, which IIRC is left-to-right, but I did not double-check.
There was a problem hiding this comment.
Note that it was actually a question. But here is another question. How universal is suggestion that * and / are left-to-right? Do you recognize that it actually depends on type? For example contemporary compilers have no problem optimizing 8*x/4 as 2*x for signed integer. But not unsigned. [Do you want to know why?] Though older gcc optimizes it as 2*x even for unsigned. And don't get me started on floating point... Of course 8*x/4 is very different from 8*x/20 and compilers should recognize that. But can you be sure? For any of compilers every compiler that are out there? And think of it even like this. Does reader of the code have to keep all those details in mind? Actual value of ENTROPY_FACTOR, dependency on type, which type is involved in operation...
There was a problem hiding this comment.
every compiler that are out there?
Well, of course one can say that it has to work with just one, VMS... Yeah, consider it a side note...
There was a problem hiding this comment.
Thanks for that heads up. Me, I'm thinking that paranoia is fine, as long as it doesn't cost our sanity. In this case, I'll leave it be, because the VMS compilers do fine with it, and considering VSI are keeping a close eye on OpenSSL, I wouldn't be surprised if it's one of the test beds for new ones.
There was a problem hiding this comment.
Fair enough. BTW, would a test fail if 0 is passed to rand_pool_add here?
There was a problem hiding this comment.
The following line would most likely return 0.
There was a problem hiding this comment.
The question was if any of the tests executed in equivalent of 'make test' would fail. I mean question is how would the keep an eye on it? They obviously won't single-step whole code base in debugger...
crypto/rand/rand_vms.c
Outdated
| * | ||
| * Output: | ||
| * items - pre-allocated ILE3 array to be filled. | ||
| * It's assume to have items_data_num elements plus |
|
This is now crash free on VMS. |
|
I'm confused. At one point you said that alignment insurance [for data_buffer_64bit] is required, but provide none. This doesn't actually contradict assertion that it's crash-free, because compiler is likely to align it, it's custom. Do you want to leave it for somebody else to keep an eye on too? Or maybe alignment is not an issue after all. You can verify this by initializing total_elems_64bit with 1 (and expanding data_buffer_64bit by 1). |
|
I totally spaced on what you might mean with "insurance", sorry. What do you have in mind? |
Contrary, this is indication that alignment is an issue. Because failure on one system is sufficient to classify it as problem. |
|
Re insurance, I separated the buffers in two, one for 64-bit data and one for 32-bit. Are you telling me that the compiler won't ensure that an individual array - actually any variable - starts at an aligned address? |
What I didn't say was that there is no error on IA64... that did take me by surprise. But I may have failed something, so I'm rebuilding from scratch with |
And problem there was that amount of preceding 32-bit values could be uneven, so that few 64-bit ones were guaranteed to be misaligned.
Compiler is obliged to align at natural type boundary. I mean 32-bit variables/arrays shall be aligned at 32-bit boundaries, 64-bit - at 64-bit. 32-bit variables can be aligned at 64-bit boundaries, and as already said, 64-bit compilers customarily align all stack objects at 64-bit boundaries. However, they don't have to, hence you can't make assumption that they actually do. Keyword here is that both arrays are declared uint32_t, even data_buffer_64bit, hence compiler may align the latter at 32 bits. So that if alignment for 64-bit values is issue, then you better arrange it in a way that makes compiler always align data_buffer_64bit at 64-bit boundary. |
|
Ah! I misremembered that as only applying within structures... It just dawned on me, btw, that there's really no need for two different data buffers, as long as we start with the 64-bit values. |
crypto/rand/rand_vms.c
Outdated
| * Try not to overfeed the pool | ||
| */ | ||
| if (total_length > bytes_remaining) | ||
| if (total_length > bytes_remaining) { |
There was a problem hiding this comment.
On a vaguely related side note one can actually wonder if this is right. I mean what's "overfeed the pool"? You don't want to overfeed the pool with entropy, right? So if you make an estimate on your source, wouldn't it be appropriate to adjust amount of data you pass down? Basically question is if "bytes_remaning" is an expression of how much entropy is missing, or is it actually how much data pool can accommodate? It's kind of ambiguous...
There was a problem hiding this comment.
bytes_remaining is the maximum available space in the pool, that's what rand_pool_bytes_remaining() returns. That and the fact that rand_pool_add() will refuse the input entirely if it's larger than the available space in the pool should answer your question.
(on might argue that rand_pool_add() should accept the amount that it can, but that's outside of the scope of this PR)
crypto/rand/rand_vms.c
Outdated
| * one extra for the terminating NULL element | ||
| * databuffer - pre-allocated 32-bit word array. | ||
| * | ||
| * Returns the number of bytes used in databuffer |
There was a problem hiding this comment.
This is no longer true. It returns number of 32-bit words.
Stop redefining structures that are already defined in system headers. This also means we can stop setting the pointer size globally, because the system structures will have the correct pointer sizes either way. The only exception is passing the right pointer size to a function. Stop trying to twist things around with rand(), that's the job of the DRBG that we feed. Stop assuming the location of the JPI$_FINALEXC item, look it up instead. Signal an exception if the sys$getjpiw call fails (it means the item list isn't set up right, so works as an assertion, but using VMS methodology). Reviewed-by: Andy Polyakov <[email protected]> (Merged from #6151)
Add more items that could serve as entropy source. Reviewed-by: Andy Polyakov <[email protected]> (Merged from #6151)
3 participants
Stop redefining structures that are already defined in system
headers. This also means we can stop setting the pointer size
globally, because the system structures will have the correct pointer
sizes either way. The only exception is passing the right pointer
size to a function.
Stop trying to twist things around with rand(), that's the job of the
DRBG that we feed.
Stop assuming the location of the JPI$_FINALEXC item, look it up
instead.
Signal an exception if the sys$getjpiw call fails (it means the item
list isn't set up right, so works as an assertion, but using VMS
methodology).
Add more items that could serve as entropy source.