kernel: prevent out-of-bound access creating finite fields #1383
Conversation
The kernel function FiniteField takes a prime <p> and a degree <d> as argument, and creates a small finite field with <q>:=<p>^<d> elements. However, it can be called with invalid arguments (e.g. where <p> is not a prime, or where <q> exceeds 2^16). It thus needs to validate its arguments, and several function calling it in fact rely on it. However, the linear interpolation search it used failed to do this properly, and thus if <q> was not a prime power, or was too large, it could end up performing out-of-bound accesses to the <SizeFF> array. Depending on the content of the memory it incorrect accessed, this could lead to an infinite loop, or to a correct error (because of some end validation), or hypothetically to nonsense computations (but only if you were *really* unlucky and the out-bounds-access resulted in *exactly* the right value). This is now fixed by this commit. In addition, after the linear interpolation search, we now verify that the <ff> index is not out-of-bounds *before* using it to access the SizeFF array. Fixes gap-system#1382
There was a problem hiding this comment.
Except for a couple of rather stylistic issues, this looks good to me. @ChrisJefferson should obviously check whether this makes #1382 go away.
| @@ -529,7 +529,8 @@ FF FiniteField ( | |||
|
|
|||
| /* search through the finite field table */ | |||
| l = 1; n = NUM_SHORT_FINITE_FIELDS; | |||
| while (l <= n) { | |||
| ff = 0; | |||
| while (l <= n && SizeFF[l] <= q && q <= SizeFF[n]) { | |||
| /* interpolation search */ | |||
There was a problem hiding this comment.
maybe brackets around the comparisons?
also maybe worthwhile break that line?
There is a macro SIZE_FF. It doesn't do anything other than SizeFF[n], but we should decide which variant we want, and use one of the two.
There was a problem hiding this comment.
I don't see a need for brackets here, and would think they'd decrease readability here.
I noticed SIZE_FF -- not sure why it exists, perhaps for historic reasons? In any case, I followed the classic approach of trying to "blend in" with the immediately surrounding code, which uses SizeFF, a lot. I didn't want to start wondering about larger issues when trying to fix a simple localized bug. I don't think it should be done within this PR.
| @@ -542,10 +543,10 @@ FF FiniteField ( | |||
| else | |||
| n = ff-1; | |||
| } | |||
| if (ff < 1 || ff > NUM_SHORT_FINITE_FIELDS) | |||
| return 0; | |||
| if (SizeFF[ff] != q) | |||
| @@ -542,10 +543,10 @@ FF FiniteField ( | |||
| else | |||
There was a problem hiding this comment.
I am somewhat a fan of {} for the ifs (in particular for the one with the else on it). I realise that this code was here before, I just noticed it)
There was a problem hiding this comment.
I don't care either way, but I am not going to add them in this PR, and also wanted to "blend in" with the existing code. It's hard enough already to stop myself from reformatting every function and file I touch ;-)
|
Obviously @ChrisJefferson should verify that -- but it certainly did make it go away for me (and I did not even have to use |
Codecov Report
@@ Coverage Diff @@
## master #1383 +/- ##
==========================================
+ Coverage 61.22% 61.26% +0.03%
==========================================
Files 1035 1035
Lines 356425 357091 +666
Branches 14225 14368 +143
==========================================
+ Hits 218223 218767 +544
- Misses 134625 134742 +117
- Partials 3577 3582 +5
|
olexandr-konovalov
added
the
release notes: added
The kernel function FiniteField takes a prime
pand a degreedasargument, and creates a small finite field with
q:=p^delements.However, it can be called with invalid arguments (e.g. where
pis not aprime, or where
qexceeds 2^16). It thus needs to validate its arguments,and several function calling it in fact rely on it.
However, the linear interpolation search it used failed to do this properly,
and thus if
qwas not a prime power, or was too large, it could end upperforming out-of-bound accesses to the
SizeFFarray. Depending on thecontent of the memory it incorrect accessed, this could lead to an infinite
loop, or to a correct error (because of some end validation), or
hypothetically to nonsense computations (but only if you were really unlucky
and the out-bounds-access resulted in exactly the right value).
This is now fixed by this commit. In addition, after the linear interpolation
search, we now verify that the
ffindex is not out-of-bounds before usingit to access the SizeFF array.
Fixes #1382