Page MenuHomeFreeBSD

UMA: unsign some variables related to allocation in hash_alloc().
ClosedPublic

Authored by pfg on Mon, Feb 11, 3:07 PM.

Details

Summary

r343673 fixed an integer overflow with huge amounts of memory by
upgrading the data type for the allocation size from an int.

It is clear that the hashsize cannot be negative so unsigning the
related variables does no harm and it should let us handle slightly
bigger values. Even if we cannot handle bigger values unsigning should
still have the advantage of simplifying the implicit casting.

Test Plan

I don't have such amount of memory to test but my kernel still works.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pfg created this revision.Mon, Feb 11, 3:07 PM
pfg added a reviewer: markj.Mon, Feb 11, 3:09 PM
mav added a comment.Mon, Feb 11, 3:44 PM

Generally I agree with it. I spotted it too when worked on my change. It just needs 8 times more RAM then what we have now to overflow, so I preferred my patch to be minimal. Just going that way, why not make uh_hashmask also unsigned? Signed bit mask makes even less sense. You've changed hval from int in one place, but not other(s).

pfg added a comment.Mon, Feb 11, 4:15 PM
In D19148#409622, @mav wrote:

Generally I agree with it. I spotted it too when worked on my change. It just needs 8 times more RAM then what we have now to overflow, so I preferred my patch to be minimal. Just going that way, why not make uh_hashmask also unsigned?

I thought of that but I saw a -1 when calculating it:

hash->uh_hashmask = hash->uh_hashsize - 1;

.A hashsize of zero makes little sense but it is stil a valid value (?).

Signed bit mask makes even less sense.  You've changed hval from int in one place, but not other(s).

OK, I caught the missing change in hash_expand(), thanks.

pfg updated this revision to Diff 53790.Mon, Feb 11, 4:20 PM

Missing hval unsigning (pointed out by mav).

mav added a comment.Mon, Feb 11, 4:29 PM
In D19148#409623, @pfg wrote:

I thought of that but I saw a -1 when calculating it:

hash->uh_hashmask = hash->uh_hashsize - 1;

.A hashsize of zero makes little sense but it is stil a valid value (?).

hashsize of zero makes no sense to me. I haven't checked how exactly it is initialized and whether it is used for some implementation reasons, but smallest possible size of hash is 1 with mask of 0. For size of 0 there is just no meaningful mask. Plus few places where uh_hashmask is used, do not care whether it is negative or overflown unsigned.

pfg added a comment.Mon, Feb 11, 10:59 PM
In D19148#409628, @mav wrote:
In D19148#409623, @pfg wrote:

I thought of that but I saw a -1 when calculating it:

hash->uh_hashmask = hash->uh_hashsize - 1;

.A hashsize of zero makes little sense but it is still a valid value (?).

hashsize of zero makes no sense to me. I haven't checked how exactly it is initialized and whether it is used for some implementation reasons, but smallest possible size of hash is 1 with mask of 0. For size of 0 there is just no meaningful mask. Plus few places where uh_hashmask is used, do not care whether it is negative or overflown unsigned.

A zero value would come from an error or a malicious user.

I am not involved with the theory here (heck ... I am a mechanical engineer not a CS guy), could we use +1 instead of -1 for the hash mask? If the idea is just to have a unique value, +1 won't overflow and having that much memory would cause other troubles anyway.

In D19148#409749, @pfg wrote:
In D19148#409628, @mav wrote:
In D19148#409623, @pfg wrote:

I thought of that but I saw a -1 when calculating it:

hash->uh_hashmask = hash->uh_hashsize - 1;

.A hashsize of zero makes little sense but it is still a valid value (?).

hashsize of zero makes no sense to me. I haven't checked how exactly it is initialized and whether it is used for some implementation reasons, but smallest possible size of hash is 1 with mask of 0. For size of 0 there is just no meaningful mask. Plus few places where uh_hashmask is used, do not care whether it is negative or overflown unsigned.

A zero value would come from an error or a malicious user.

I don't see how that can happen. This functionality is entirely internal to the kernel.

I am not involved with the theory here (heck ... I am a mechanical engineer not a CS guy), could we use +1 instead of -1 for the hash mask? If the idea is just to have a unique value, +1 won't overflow and having that much memory would cause other troubles anyway.

No, that would break the algorithm. The idea is to create a mask by subtracting 1 from a power of 2. I agree with mav that there's no reason to have a signed mask.

sys/vm/uma_core.c
683 ↗(On Diff #53790)

The diff should avoid mixing style and functional changes.

pfg updated this revision to Diff 53818.Tue, Feb 12, 12:33 AM

Unsign uh_hashmask
(Don't expect any failure but will test anyways)

mav accepted this revision.Tue, Feb 12, 12:43 AM
This revision is now accepted and ready to land.Tue, Feb 12, 12:43 AM
This revision was automatically updated to reflect the committed changes.