search

aws / s2n-tls

An implementation of the TLS/SSL protocols
https://aws.github.io/s2n-tls/usage-guide/
Apache License 2.0
4.53k stars 705 forks source link

munlock should take allocate, not size. #1502

Closed danielsn closed 4 years ago

danielsn commented 4 years ago

Problem:

https://github.com/awslabs/s2n/blob/master/utils/s2n_mem.c#L118 We leak locked pages, since the size may be less than the allocated bytes

Proposed Solution:

Use allocated here.

danielsn commented 4 years ago

munlock() unlocks pages in the address range starting at addr and continuing for len bytes. After this call, all pages that contain a part of the specified memory range can be moved to external swap space again by the kernel.

https://linux.die.net/man/2/munlock

maddeleine commented 4 years ago

We only mlock on the size of the data, not the amount allocated (L72). So it seems like L118 is technically correct, but we should update mlock in s2n_realloc to reflect the new size of the data in this check: L93. Maybe either way works, because it might not be that big of a deal if we munlock more than we mlock.

danielsn commented 4 years ago

Yeah, we can either munlock(blob->size), then mlock(newsize) in the realloc function every time its called, or we could just mlock(allocated) when memory is aquired, and munlock(allocated) when its free. I personally find the second clearer, because then all our operations happen on page boundaries. But YMMV.

There is also possible a performance cost. Each mlock or munlock call requires a kernel call. So it may be expensive to do more than necessary. Profiling would be the best way to tell.