From ff62700fa8ee717a71de13baec25f0d69640ae7c Mon Sep 17 00:00:00 2001 From: Andreas Dilger Date: Thu, 31 Aug 2023 14:50:56 -0600 Subject: [PATCH] LU-17000 ptlrpc: fix string overflow warnings Fix potential string overflow warnings in sptlrpc_flavor2name() calling strncat() with the full size of the target buffer instead of the *remaining* space in the target buffer. Fix potential string overflow warning in sepol_seq_write_old() and sepol_seq_write() potentially copying an unterminated string from userspace via strncpy() and not terminating it afterward. Since the maximum incoming parameter size is known in advance, is reasonably small (~342 bytes), and is only used temporarily, reorganize the code to avoid two buffer allocations and copies. Use memcpy() to copy the string since its length is known, and always add a NUL terminator to the string afterward. Improvements to error messages and code style in these functions. Addresses-Coverity: 199034 ("Out-of-bounds access") Addresses-Coverity: 199063 ("Out-of-bounds access") Addresses-Coverity: 199108 ("Out-of-bounds access") Addresses-Coverity: 397374 ("String not null terminated") Addresses-Coverity: 397394 ("String not null terminated") Test-Parameters: trivial testlist=sanity-sec,sanity-selinux Signed-off-by: Andreas Dilger Change-Id: Ia810ce9f07b663a90049bb78af21c06f0e3ebbe5 Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/52210 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Sebastien Buisson Reviewed-by: Timothy Day Reviewed-by: Arshad Hussain Reviewed-by: Oleg Drokin --- lustre/ptlrpc/sec.c | 8 +- lustre/ptlrpc/sec_lproc.c | 186 ++++++++++++++++++++-------------------------- 2 files changed, 86 insertions(+), 108 deletions(-) diff --git a/lustre/ptlrpc/sec.c b/lustre/ptlrpc/sec.c index ab747f8..50905f2 100644 --- a/lustre/ptlrpc/sec.c +++ b/lustre/ptlrpc/sec.c @@ -230,7 +230,9 @@ EXPORT_SYMBOL(sptlrpc_flavor2name_bulk); char *sptlrpc_flavor2name(struct sptlrpc_flavor *sf, char *buf, int bufsize) { - snprintf(buf, bufsize, "%s", sptlrpc_flavor2name_base(sf->sf_rpc)); + size_t ln; + + ln = snprintf(buf, bufsize, "%s", sptlrpc_flavor2name_base(sf->sf_rpc)); /* * currently we don't support customized bulk specification for @@ -240,8 +242,8 @@ char *sptlrpc_flavor2name(struct sptlrpc_flavor *sf, char *buf, int bufsize) char bspec[16]; bspec[0] = '-'; - sptlrpc_flavor2name_bulk(sf, &bspec[1], sizeof(bspec) - 1); - strncat(buf, bspec, bufsize); + sptlrpc_flavor2name_bulk(sf, bspec + 1, sizeof(bspec) - 1); + strncat(buf, bspec, bufsize - ln); } buf[bufsize - 1] = '\0'; diff --git a/lustre/ptlrpc/sec_lproc.c b/lustre/ptlrpc/sec_lproc.c index c6c0242..a9e2861 100644 --- a/lustre/ptlrpc/sec_lproc.c +++ b/lustre/ptlrpc/sec_lproc.c @@ -79,12 +79,12 @@ static int sptlrpc_info_lprocfs_seq_show(struct seq_file *seq, void *v) strcmp(obd->obd_type->typ_name, LUSTRE_LWP_NAME) == 0 || strcmp(obd->obd_type->typ_name, LUSTRE_OSP_NAME) == 0); - if (cli->cl_import) - sec = sptlrpc_import_sec_ref(cli->cl_import); - if (sec == NULL) - goto out; + if (cli->cl_import) + sec = sptlrpc_import_sec_ref(cli->cl_import); + if (sec == NULL) + goto out; - sec_flags2str(sec->ps_flvr.sf_flags, str, sizeof(str)); + sec_flags2str(sec->ps_flvr.sf_flags, str, sizeof(str)); seq_printf(seq, "rpc flavor: %s\n", sptlrpc_flavor2name_base(sec->ps_flvr.sf_rpc)); @@ -103,7 +103,7 @@ static int sptlrpc_info_lprocfs_seq_show(struct seq_file *seq, void *v) sptlrpc_sec_put(sec); out: - return 0; + return 0; } LDEBUGFS_SEQ_FOPS_RO(sptlrpc_info_lprocfs); @@ -120,45 +120,46 @@ static int sptlrpc_ctxs_lprocfs_seq_show(struct seq_file *seq, void *v) strcmp(obd->obd_type->typ_name, LUSTRE_LWP_NAME) == 0 || strcmp(obd->obd_type->typ_name, LUSTRE_OSP_NAME) == 0); - if (cli->cl_import) - sec = sptlrpc_import_sec_ref(cli->cl_import); - if (sec == NULL) - goto out; + if (cli->cl_import) + sec = sptlrpc_import_sec_ref(cli->cl_import); + if (sec == NULL) + goto out; - if (sec->ps_policy->sp_cops->display) - sec->ps_policy->sp_cops->display(sec, seq); + if (sec->ps_policy->sp_cops->display) + sec->ps_policy->sp_cops->display(sec, seq); - sptlrpc_sec_put(sec); + sptlrpc_sec_put(sec); out: - return 0; + return 0; } LDEBUGFS_SEQ_FOPS_RO(sptlrpc_ctxs_lprocfs); #if LUSTRE_VERSION_CODE < OBD_OCD_VERSION(2, 16, 53, 0) static ssize_t sepol_seq_write_old(struct obd_device *obd, - const char __user *buffer, - size_t count) + const char __user *buffer, size_t count) { struct client_obd *cli = &obd->u.cli; struct obd_import *imp = cli->cl_import; struct sepol_downcall_data_old *param; - int size = sizeof(*param); - __u16 len; + size_t maxlen = sizeof(imp->imp_sec->ps_sepol); + size_t size = sizeof(*param); + size_t maxparam = sizeof(*param) + maxlen; + int len; int rc = 0; - if (count < size) { + if (count <= size) { rc = -EINVAL; - CERROR("%s: invalid data count = %lu, size = %d: rc = %d\n", - obd->obd_name, (unsigned long) count, size, rc); + CERROR("%s: invalid data count %zu <= size %zu: rc = %d\n", + obd->obd_name, count, size, rc); return rc; } - OBD_ALLOC(param, size); - if (param == NULL) + OBD_ALLOC(param, maxparam); + if (!param) return -ENOMEM; - if (copy_from_user(param, buffer, size)) { + if (copy_from_user(param, buffer, min(count, maxparam))) { rc = -EFAULT; CERROR("%s: bad sepol data: rc = %d\n", obd->obd_name, rc); GOTO(out, rc); @@ -166,53 +167,38 @@ static ssize_t sepol_seq_write_old(struct obd_device *obd, if (param->sdd_magic != SEPOL_DOWNCALL_MAGIC_OLD) { rc = -EINVAL; - CERROR("%s: sepol downcall bad params: rc = %d\n", - obd->obd_name, rc); + CERROR("%s: sepol downcall bad magic %#08x != %#08x: rc = %d\n", + obd->obd_name, param->sdd_magic, + SEPOL_DOWNCALL_MAGIC_OLD, rc); GOTO(out, rc); } - if (param->sdd_sepol_len == 0 || - param->sdd_sepol_len >= sizeof(imp->imp_sec->ps_sepol)) { + len = param->sdd_sepol_len; + if (len == 0 || len >= maxlen) { rc = -EINVAL; - CERROR("%s: invalid sepol data returned: rc = %d\n", - obd->obd_name, rc); + CERROR("%s: bad sepol len %u >= maxlen %zu: rc = %d\n", + obd->obd_name, len, maxlen, rc); GOTO(out, rc); } - len = param->sdd_sepol_len; /* save sdd_sepol_len */ - OBD_FREE(param, size); - size = offsetof(struct sepol_downcall_data_old, - sdd_sepol[len]); + size = offsetof(typeof(*param), sdd_sepol[len]); if (count < size) { rc = -EINVAL; - CERROR("%s: invalid sepol count = %lu, size = %d: rc = %d\n", - obd->obd_name, (unsigned long) count, size, rc); - return rc; - } - - /* alloc again with real size */ - OBD_ALLOC(param, size); - if (param == NULL) - return -ENOMEM; - - if (copy_from_user(param, buffer, size)) { - rc = -EFAULT; - CERROR("%s: cannot copy sepol data: rc = %d\n", - obd->obd_name, rc); + CERROR("%s: bad sepol count %zu < total size %zu: rc = %d\n", + obd->obd_name, count, size, rc); GOTO(out, rc); } spin_lock(&imp->imp_sec->ps_lock); - snprintf(imp->imp_sec->ps_sepol, param->sdd_sepol_len + 1, "%s", - param->sdd_sepol); + memcpy(imp->imp_sec->ps_sepol, param->sdd_sepol, len); + imp->imp_sec->ps_sepol[len + 1] = '\0'; imp->imp_sec->ps_sepol_mtime = ktime_set(param->sdd_sepol_mtime, 0); spin_unlock(&imp->imp_sec->ps_lock); out: - if (param != NULL) - OBD_FREE(param, size); + OBD_FREE(param, maxparam); - return rc ? rc : count; + return rc ?: count; } #endif @@ -225,89 +211,79 @@ ldebugfs_sptlrpc_sepol_seq_write(struct file *file, const char __user *buffer, struct client_obd *cli = &obd->u.cli; struct obd_import *imp = cli->cl_import; struct sepol_downcall_data *param; - __u32 magic; - int size = sizeof(magic); - __u16 len; + size_t maxlen = sizeof(imp->imp_sec->ps_sepol); + size_t size = sizeof(*param); + size_t maxparam = size + maxlen; + int len; int rc = 0; - if (count < size) { + if (count <= size) { rc = -EINVAL; - CERROR("%s: invalid buffer count = %lu, size = %d: rc = %d\n", - obd->obd_name, (unsigned long) count, size, rc); - return rc; - } - - if (copy_from_user(&magic, buffer, size)) { - rc = -EFAULT; - CERROR("%s: bad sepol magic: rc = %d\n", obd->obd_name, rc); + CERROR("%s: invalid data count %zu <= size %zu: rc = %d\n", + obd->obd_name, count, size, rc); return rc; } - if (magic != SEPOL_DOWNCALL_MAGIC) { #if LUSTRE_VERSION_CODE < OBD_OCD_VERSION(2, 16, 53, 0) - if (magic == SEPOL_DOWNCALL_MAGIC_OLD) { - return sepol_seq_write_old(obd, buffer, count); + { + __u32 magic; + + if (copy_from_user(&magic, buffer, sizeof(magic))) { + rc = -EFAULT; + CERROR("%s: bad sepol magic data: rc = %d\n", + obd->obd_name, rc); + return rc; } -#endif - rc = -EINVAL; - CERROR("%s: sepol downcall bad magic '%#08x': rc = %d\n", - obd->obd_name, magic, rc); - return rc; - } - size = sizeof(*param); - if (count < size) { - rc = -EINVAL; - CERROR("%s: invalid data count = %lu, size = %d: rc = %d\n", - obd->obd_name, (unsigned long) count, size, rc); - return rc; + if (unlikely(magic == SEPOL_DOWNCALL_MAGIC_OLD)) + return sepol_seq_write_old(obd, buffer, count); } +#endif - OBD_ALLOC(param, size); - if (param == NULL) + OBD_ALLOC(param, maxparam); + if (!param) return -ENOMEM; - if (copy_from_user(param, buffer, size)) { + if (copy_from_user(param, buffer, min(count, maxparam))) { rc = -EFAULT; CERROR("%s: bad sepol data: rc = %d\n", obd->obd_name, rc); GOTO(out, rc); } - if (param->sdd_sepol_len == 0 || - param->sdd_sepol_len >= sizeof(imp->imp_sec->ps_sepol)) { + if (param->sdd_magic != SEPOL_DOWNCALL_MAGIC) { rc = -EINVAL; - CERROR("%s: invalid sepol data returned: rc = %d\n", - obd->obd_name, rc); + CERROR("%s: sepol downcall bad magic %#08x != %#08x: rc = %d\n", + obd->obd_name, param->sdd_magic, + SEPOL_DOWNCALL_MAGIC, rc); GOTO(out, rc); } - len = param->sdd_sepol_len; /* save sdd_sepol_len */ - OBD_FREE(param, size); - size = offsetof(struct sepol_downcall_data, - sdd_sepol[len]); - - /* alloc again with real size */ - OBD_ALLOC(param, size); - if (param == NULL) - return -ENOMEM; - if (copy_from_user(param, buffer, size)) { - rc = -EFAULT; - CERROR("%s: cannot copy sepol data: rc = %d\n", - obd->obd_name, rc); + len = param->sdd_sepol_len; + if (len == 0 || len >= maxlen) { + rc = -EINVAL; + CERROR("%s: bad sepol len %u >= maxlen %zu: rc = %d\n", + obd->obd_name, len, maxlen, rc); + GOTO(out, rc); + } + size = offsetof(typeof(*param), sdd_sepol[len]); + + if (count < size) { + rc = -EINVAL; + CERROR("%s: bad sepol count %zu < total size %zu: rc = %d\n", + obd->obd_name, count, size, rc); GOTO(out, rc); } spin_lock(&imp->imp_sec->ps_lock); - snprintf(imp->imp_sec->ps_sepol, param->sdd_sepol_len + 1, "%s", - param->sdd_sepol); + memcpy(imp->imp_sec->ps_sepol, param->sdd_sepol, len); + imp->imp_sec->ps_sepol[len + 1] = '\0'; imp->imp_sec->ps_sepol_mtime = ktime_set(param->sdd_sepol_mtime, 0); spin_unlock(&imp->imp_sec->ps_lock); out: - if (param != NULL) - OBD_FREE(param, size); + OBD_FREE(param, maxparam); - return rc ? rc : count; + return rc ?: count; } LDEBUGFS_FOPS_WR_ONLY(srpc, sptlrpc_sepol); -- 1.8.3.1