From 7cc5cb8083f427d93afaaf4409f36a6f9f35acab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20F=C3=BCl=C3=B6p?= Date: Thu, 21 Oct 2021 12:17:47 +0200 Subject: [PATCH] pam_zfs_key: malloc and mlock/munlock won't match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mlock(2) and munlock(2) operate on memory pages whereas malloc(3) does not. So if you munlock(2) a malloced memory region, the whole page containing it is freed. Since this page may contain another malloced and mlocked memory region, used as a password buffer by a concurrent running instance of pam_zfs_key, there is a slight chance of leaking passwords. By using mmap(2) we avoid such problems since it will return whole pages on page aligned addresses. Although the above concern may be mostly academical, it is still better to use mmap(2) for allocating memory since the FreeBSD documentation suggests to call mlock(2) and munlock(2) on page aligned addresses, and other implementations even require it. While here, remove duplicate code in alloc_pw_string() by calling alloc_pw_size(). Reviewed-by: Felix Dörre Reviewed-by: Brian Behlendorf Signed-off-by: Attila Fülöp Closes #12665 --- contrib/pam_zfs_key/pam_zfs_key.c | 48 ++++++++++++++----------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/contrib/pam_zfs_key/pam_zfs_key.c b/contrib/pam_zfs_key/pam_zfs_key.c index e4196b7ad0ff..dead090f9701 100644 --- a/contrib/pam_zfs_key/pam_zfs_key.c +++ b/contrib/pam_zfs_key/pam_zfs_key.c @@ -41,6 +41,7 @@ #if defined(__linux__) #include +#define MAP_FLAGS MAP_PRIVATE | MAP_ANONYMOUS #elif defined(__FreeBSD__) #include static void @@ -51,6 +52,7 @@ pam_syslog(pam_handle_t *pamh, int loglevel, const char *fmt, ...) vsyslog(loglevel, fmt, args); va_end(args); } +#define MAP_FLAGS MAP_PRIVATE | MAP_ANON | MAP_NOCORE #endif #include @@ -77,9 +79,9 @@ typedef struct { } pw_password_t; /* - * Try to mlock or munlock addr while handling EAGAIN by retrying ten times - * and sleeping 10 milliseconds in between for a total of 0.1 seconds. - * lock_func must point to either mlock(2) or munlock(2). + * Try to mlock(2) or munlock(2) addr while handling EAGAIN by retrying ten + * times and sleeping 10 milliseconds in between for a total of 0.1 + * seconds. lock_func must point to either mlock(2) or munlock(2). */ static int try_lock(mlock_func_t lock_func, const void *addr, size_t len) @@ -110,18 +112,25 @@ alloc_pw_size(size_t len) } pw->len = len; /* - * The use of malloc() triggers a spurious gcc 11 -Wmaybe-uninitialized - * warning in the mlock() function call below, so use calloc(). + * We use mmap(2) rather than malloc(3) since later on we mlock(2) the + * memory region. Since mlock(2) and munlock(2) operate on whole memory + * pages we should allocate a whole page here as mmap(2) does. Further + * this ensures that the addresses passed to mlock(2) an munlock(2) are + * on a page boundary as suggested by FreeBSD and required by some + * other implementations. Finally we avoid inadvertently munlocking + * memory mlocked by an concurrently running instance of us. */ - pw->value = calloc(len, 1); - if (!pw->value) { + pw->value = mmap(NULL, pw->len, PROT_READ | PROT_WRITE, MAP_FLAGS, + -1, 0); + + if (pw->value == MAP_FAILED) { free(pw); return (NULL); } if (try_lock(mlock, pw->value, pw->len) != 0) { - free(pw->value); + (void) munmap(pw->value, pw->len); free(pw); - return NULL; + return (NULL); } return (pw); } @@ -129,25 +138,12 @@ alloc_pw_size(size_t len) static pw_password_t * alloc_pw_string(const char *source) { - pw_password_t *pw = malloc(sizeof (pw_password_t)); + size_t len = strlen(source) + 1; + pw_password_t *pw = alloc_pw_size(len); + if (!pw) { return (NULL); } - pw->len = strlen(source) + 1; - /* - * The use of malloc() triggers a spurious gcc 11 -Wmaybe-uninitialized - * warning in the mlock() function call below, so use calloc(). - */ - pw->value = calloc(pw->len, 1); - if (!pw->value) { - free(pw); - return (NULL); - } - if (try_lock(mlock, pw->value, pw->len) != 0) { - free(pw->value); - free(pw); - return NULL; - } memcpy(pw->value, source, pw->len); return (pw); } @@ -157,7 +153,7 @@ pw_free(pw_password_t *pw) { bzero(pw->value, pw->len); if (try_lock(munlock, pw->value, pw->len) == 0) { - free(pw->value); + (void) munmap(pw->value, pw->len); } free(pw); }