Page MenuHomeFreeBSD

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

Authored by pfg on Feb 11 2019, 3:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 19, 10:40 AM
Unknown Object (File)
Tue, Mar 19, 10:39 AM
Unknown Object (File)
Thu, Feb 29, 5:28 AM
Unknown Object (File)
Feb 18 2024, 8:04 PM
Unknown Object (File)
Jan 7 2024, 7:31 AM
Unknown Object (File)
Jan 7 2024, 7:31 AM
Unknown Object (File)
Jan 2 2024, 6:34 AM
Unknown Object (File)
Dec 22 2023, 11:37 PM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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).

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.

Missing hval unsigning (pointed out by mav).

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.

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.

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

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