_POSIX2_LINE_MAX is 2048. Allocate the buffers on the heap instead. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/bhyve/bhyve_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 80d5a8804f..c8e1a10afe 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -380,7 +380,7 @@ virBhyveGetDomainTotalCpuStats(virDomainObj *vm, { struct kinfo_proc *kp; kvm_t *kd; - char errbuf[_POSIX2_LINE_MAX]; + g_autofree char *errbuf = g_new0(char, _POSIX2_LINE_MAX); int nprocs; int ret = -1; @@ -481,7 +481,7 @@ virBhyveProcessReconnectAll(struct _bhyveConn *driver) { kvm_t *kd; struct bhyveProcessReconnectData data; - char errbuf[_POSIX2_LINE_MAX]; + g_autofree char *errbuf = g_new0(char, _POSIX2_LINE_MAX); if ((kd = kvm_openfiles(NULL, NULL, NULL, O_RDONLY, errbuf)) == NULL) { virReportError(VIR_ERR_SYSTEM_ERROR, -- 2.41.0

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tools/virt-host-validate-bhyve.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/virt-host-validate-bhyve.c b/tools/virt-host-validate-bhyve.c index a39225016d..9457fac9a5 100644 --- a/tools/virt-host-validate-bhyve.c +++ b/tools/virt-host-validate-bhyve.c @@ -50,8 +50,10 @@ int virHostValidateBhyve(void) int ret = 0; int fileid = 0; struct kld_file_stat stat; - bool vmm_loaded = false, if_tap_loaded = false; - bool if_bridge_loaded = false, nmdm_loaded = false; + bool vmm_loaded = false; + bool if_tap_loaded = false; + bool if_bridge_loaded = false; + bool nmdm_loaded = false; for (fileid = kldnext(0); fileid > 0; fileid = kldnext(fileid)) { stat.version = sizeof(struct kld_file_stat); -- 2.41.0

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tools/virt-host-validate-bhyve.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/virt-host-validate-bhyve.c b/tools/virt-host-validate-bhyve.c index 9457fac9a5..db1cdd8e2c 100644 --- a/tools/virt-host-validate-bhyve.c +++ b/tools/virt-host-validate-bhyve.c @@ -49,24 +49,24 @@ int virHostValidateBhyve(void) { int ret = 0; int fileid = 0; - struct kld_file_stat stat; + g_autofree struct kld_file_stat *stat = g_new0(struct kld_file_stat, 1); bool vmm_loaded = false; bool if_tap_loaded = false; bool if_bridge_loaded = false; bool nmdm_loaded = false; for (fileid = kldnext(0); fileid > 0; fileid = kldnext(fileid)) { - stat.version = sizeof(struct kld_file_stat); - if (kldstat(fileid, &stat) < 0) + stat->version = sizeof(struct kld_file_stat); + if (kldstat(fileid, stat) < 0) continue; - if (STREQ(stat.name, "vmm.ko")) + if (STREQ(stat->name, "vmm.ko")) vmm_loaded = true; - else if (STREQ(stat.name, "if_tap.ko")) + else if (STREQ(stat->name, "if_tap.ko")) if_tap_loaded = true; - else if (STREQ(stat.name, "if_bridge.ko")) + else if (STREQ(stat->name, "if_bridge.ko")) if_bridge_loaded = true; - else if (STREQ(stat.name, "nmdm.ko")) + else if (STREQ(stat->name, "nmdm.ko")) nmdm_loaded = true; } -- 2.41.0

Break up the argument and variable declarations to the preferred style. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tools/nss/libvirt_nss.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index b028f28608..cd2b8feb5a 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -464,13 +464,19 @@ ns_mtab methods[] = { }; static void -aiforaf(const char *name, int af, struct addrinfo *pai, struct addrinfo **aip) +aiforaf(const char *name, + int af, + struct addrinfo *pai, + struct addrinfo **aip) { int ret; struct hostent resolved; char buf[1024] = { 0 }; - int err, herr; - struct addrinfo hints, *res0, *res; + int err; + int herr; + struct addrinfo hints; + struct addrinfo *res0; + struct addrinfo *res; char **addrList; if ((ret = NSS_NAME(gethostbyname2)(name, af, &resolved, -- 2.41.0

The variable is set but never actually used. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tools/nss/libvirt_nss.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index cd2b8feb5a..faa44e78df 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -469,7 +469,6 @@ aiforaf(const char *name, struct addrinfo *pai, struct addrinfo **aip) { - int ret; struct hostent resolved; char buf[1024] = { 0 }; int err; @@ -479,9 +478,9 @@ aiforaf(const char *name, struct addrinfo *res; char **addrList; - if ((ret = NSS_NAME(gethostbyname2)(name, af, &resolved, - buf, sizeof(buf), - &err, &herr)) != NS_SUCCESS) + if (NSS_NAME(gethostbyname2)(name, af, &resolved, + buf, sizeof(buf), + &err, &herr) != NS_SUCCESS) return; addrList = resolved.h_addr_list; -- 2.41.0

The 'port' buffer is passed to 'getnameinfo' which is supposed to fill it but it's not actually later used. Drop the buffer as 'getnameinfo' allows NULL arguments if they are not needed. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tools/nss/libvirt_nss.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index faa44e78df..37720bf4ae 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -493,7 +493,6 @@ aiforaf(const char *name, socklen_t salen; void *address = *addrList; char host[NI_MAXHOST]; - char port[NI_MAXSERV]; if (resolved.h_addrtype == AF_INET) { sa.sin.sin_family = AF_INET; @@ -511,7 +510,7 @@ aiforaf(const char *name, if ((err = getnameinfo(&sa.sa, salen, host, sizeof(host), - port, sizeof(port), + NULL, 0, NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { ERROR("Cannot convert socket address to string: %s", gai_strerror(err)); -- 2.41.0

The 'buf', 'sa' and 'hints' stack allocated helper variables are never used together. Decrease the stack memory usage by scoping them off into do-while blocks. In this instance we do not want to use dynamic allocation as this is the NSS module. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tools/nss/libvirt_nss.c | 97 +++++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 43 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 37720bf4ae..dff3c034bf 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -470,61 +470,72 @@ aiforaf(const char *name, struct addrinfo **aip) { struct hostent resolved; - char buf[1024] = { 0 }; int err; - int herr; - struct addrinfo hints; - struct addrinfo *res0; - struct addrinfo *res; char **addrList; - if (NSS_NAME(gethostbyname2)(name, af, &resolved, - buf, sizeof(buf), - &err, &herr) != NS_SUCCESS) - return; + /* Note: The do-while blocks in this function are used to scope off large + * stack allocated buffers, which are not needed at the same time */ + do { + char buf[1024] = { 0 }; + int herr; + + if (NSS_NAME(gethostbyname2)(name, af, &resolved, + buf, sizeof(buf), + &err, &herr) != NS_SUCCESS) + return; + } while (false); addrList = resolved.h_addr_list; while (*addrList) { - union { - struct sockaddr sa; - struct sockaddr_in sin; - struct sockaddr_in6 sin6; - } sa = { 0 }; - socklen_t salen; void *address = *addrList; char host[NI_MAXHOST]; + struct addrinfo *res0; + struct addrinfo *res; + + do { + union { + struct sockaddr sa; + struct sockaddr_in sin; + struct sockaddr_in6 sin6; + } sa = { 0 }; + socklen_t salen; + + if (resolved.h_addrtype == AF_INET) { + sa.sin.sin_family = AF_INET; + memcpy(&sa.sin.sin_addr.s_addr, + address, + FAMILY_ADDRESS_SIZE(AF_INET)); + salen = sizeof(sa.sin); + } else { + sa.sin6.sin6_family = AF_INET6; + memcpy(&sa.sin6.sin6_addr.s6_addr, + address, + FAMILY_ADDRESS_SIZE(AF_INET6)); + salen = sizeof(sa.sin6); + } - if (resolved.h_addrtype == AF_INET) { - sa.sin.sin_family = AF_INET; - memcpy(&sa.sin.sin_addr.s_addr, - address, - FAMILY_ADDRESS_SIZE(AF_INET)); - salen = sizeof(sa.sin); - } else { - sa.sin6.sin6_family = AF_INET6; - memcpy(&sa.sin6.sin6_addr.s6_addr, - address, - FAMILY_ADDRESS_SIZE(AF_INET6)); - salen = sizeof(sa.sin6); - } + if ((err = getnameinfo(&sa.sa, salen, + host, sizeof(host), + NULL, 0, + NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { + ERROR("Cannot convert socket address to string: %s", + gai_strerror(err)); + continue; + } + } while (false); - if ((err = getnameinfo(&sa.sa, salen, - host, sizeof(host), - NULL, 0, - NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - ERROR("Cannot convert socket address to string: %s", - gai_strerror(err)); - continue; - } + do { + struct addrinfo hints; - hints = *pai; - hints.ai_flags = AI_NUMERICHOST; - hints.ai_family = af; + hints = *pai; + hints.ai_flags = AI_NUMERICHOST; + hints.ai_family = af; - if (getaddrinfo(host, NULL, &hints, &res0)) { - addrList++; - continue; - } + if (getaddrinfo(host, NULL, &hints, &res0)) { + addrList++; + continue; + } + } while (false); for (res = res0; res; res = res->ai_next) res->ai_flags = pai->ai_flags; -- 2.41.0

On Mon, Sep 04, 2023 at 16:09:00 +0200, Michal Prívozník wrote: The main idea of this is to ensure that we don't have massive stack frames which could cause a stack overflow. But to be honest, it doesn't matter too much whether the actual limit size is 4k or 2k. It still allows us to have very deep call stack, and a factor of 2 in the size will not make a real difference. At one point I was looking at which functions have massive stack frames and tried avoiding such state. Now this last set of patches was what I had in my local branch for a long time and decided to finally get rid of them. As you can see, there were multiple cases of 2k buffers being stack allocated, so the end result IMO made sense. The decreasing of the actual limit to 2k isn't as important as I've noted but similarly to when we add a syntax check after a full-repo cleanup it is a way to prevent regressions after a cleanup.

On 9/5/23 09:43, Peter Krempa wrote: I'm failing to see what stack size and buffer overflow have in common. To overwrite the return address you can has as small buffer as 1 byte and not check the boundaries (obviously). And yet, you can have 2K buffer on stack but if you check the boundaries when reading into it, no exploit is possible. Yeah. If we care about the actual stack usage (e.g. like kernel does - it has limit of 8KiB for the whole stack of a kernel thread), then restricting individual functions is not enough since we have recursive functions. Indeed and this reason alone is good enough. It made the code cleaner too. I was just wondering whether there is some deeper sense. BTW: if you (or anybody else) want to identify functions which have massive stack usage, I've found clever perl script from kernel.git: libvirt.git $ objdump -d _build/src/libvirt.so.0.9008.0 | \ kernel.git/scripts/checkstack.pl | \ head -n30 some very interesting functions appear on the list (virDomainDefFeaturesCheckABIStability()) - but when looking at the disassembled code directly, it's not just about initial sub 0x68, %rsp; it's also about how many arguments are passed to functions (esp. with variable arguments) Michal