From d09134f6edeff8d445cb96ed3689f4d072a9666f Mon Sep 17 00:00:00 2001 From: Hauke Mehrtens Date: Thu, 17 Sep 2026 21:25:02 +0200 Subject: [PATCH] nvram: bound the variable parser to the nvram partition The "name=value\0 ... \0\0" walk over the mmap()ed nvram used strchr() and strlen() without a bound, so an nvram without the terminating double NUL made it run past the end of the mapping. Search with memchr() up to the length from the nvram header, or up to the end of the mapping when that length does not fit, so a broken header is still readable. A variable that is not terminated in this area ends the parsing. Map a regular file, like the /tmp staging file, only as far as it is backed, rounded down to 4 bytes for the padding in nvram_commit(), instead of the partition size which raised SIGBUS. Only search the start of a mapping smaller than NVRAM_MIN_SPACE for the magic instead of underflowing the loop boundary; bcm47xxpart creates such nvram partitions on flashes with 4 KiB erase blocks. "nvram info" clamps the header length before the CRC8, it read beyond the mapping or underflowed into a 4 GB read before. This is reachable with a crafted nvram partition or staging file, on bcm47xx the nvram partition is also parsed while booting. Reported by @tonoyx91 and @0xROI. Link: https://github.com/openwrt/openwrt/security/advisories/GHSA-5m25-qmhm-crr4 Fixes: 400c1a7fbfad ("Add unvram, a nvram manipulation tool suitable for brcm-2.4 and bcm47xx platforms") Assisted-by: Claude:claude-opus-5 Link: https://github.com/openwrt/openwrt/pull/25240 Signed-off-by: Hauke Mehrtens --- package/utils/nvram/Makefile | 2 +- package/utils/nvram/src/cli.c | 19 ++++++-- package/utils/nvram/src/nvram.c | 78 +++++++++++++++++++++++++++------ 3 files changed, 81 insertions(+), 18 deletions(-) diff --git a/package/utils/nvram/Makefile b/package/utils/nvram/Makefile index 5430c97dc11..e54d3cfb9a7 100644 --- a/package/utils/nvram/Makefile +++ b/package/utils/nvram/Makefile @@ -8,7 +8,7 @@ include $(TOPDIR)/rules.mk PKG_NAME:=nvram -PKG_RELEASE:=14 +PKG_RELEASE:=15 PKG_BUILD_DIR := $(BUILD_DIR)/$(PKG_NAME) diff --git a/package/utils/nvram/src/cli.c b/package/utils/nvram/src/cli.c index c3ab105a0ed..520990b3be9 100644 --- a/package/utils/nvram/src/cli.c +++ b/package/utils/nvram/src/cli.c @@ -107,10 +107,21 @@ static int do_set(nvram_handle_t *nvram, const char *pair) static int do_info(nvram_handle_t *nvram) { nvram_header_t *hdr = nvram_header(nvram); + uint32_t len = hdr->len; + uint8_t crc; + + /* + * The length is taken from the nvram, do not use it to read beyond + * the mapped partition or to underflow the calculations below. + */ + if (len > nvram->length - nvram->offset) + len = nvram->length - nvram->offset; + if (len < NVRAM_CRC_START_POSITION) + len = NVRAM_CRC_START_POSITION; /* CRC8 over the last 11 bytes of the header and data bytes */ - uint8_t crc = hndcrc8((unsigned char *) &hdr[0] + NVRAM_CRC_START_POSITION, - hdr->len - NVRAM_CRC_START_POSITION, 0xff); + crc = hndcrc8((unsigned char *) &hdr[0] + NVRAM_CRC_START_POSITION, + len - NVRAM_CRC_START_POSITION, 0xff); /* Show info */ printf("Magic: 0x%08X\n", hdr->magic); @@ -127,8 +138,8 @@ static int do_info(nvram_handle_t *nvram) printf("NCDL values: 0x%08X\n\n", hdr->config_ncdl); printf("%i bytes used / %i bytes available (%.2f%%)\n", - hdr->len, nvram->length - nvram->offset - hdr->len, - (100.00 / (double)(nvram->length - nvram->offset)) * (double)hdr->len); + len, nvram->length - nvram->offset - len, + (100.00 / (double)(nvram->length - nvram->offset)) * (double)len); return 0; } diff --git a/package/utils/nvram/src/nvram.c b/package/utils/nvram/src/nvram.c index f2a4d0589f2..f67024f0c96 100644 --- a/package/utils/nvram/src/nvram.c +++ b/package/utils/nvram/src/nvram.c @@ -101,7 +101,7 @@ static nvram_tuple_t * _nvram_realloc( nvram_handle_t *h, nvram_tuple_t *t, static int _nvram_rehash(nvram_handle_t *h) { nvram_header_t *header = nvram_header(h); - char buf[] = "0xXXXXXXXX", *name, *value, *eq; + char buf[] = "0xXXXXXXXX", *name, *value, *eq, *nul, *end; /* (Re)initialize hash table */ _nvram_free(h); @@ -109,13 +109,33 @@ static int _nvram_rehash(nvram_handle_t *h) /* Parse and set "name=value\0 ... \0\0" */ name = (char *) &header[1]; - for (; *name; name = value + strlen(value) + 1) { - if (!(eq = strchr(name, '='))) + /* + * Stop at the end of the used area, but never look beyond the mapped + * partition. A length which does not fit the partition is ignored to + * still read the variables of a broken nvram. + */ + end = h->mmap + h->length; + if (header->len >= sizeof(nvram_header_t) && + header->len <= h->length - h->offset) + end = (char *) header + header->len; + + while (name < end && *name) { + eq = memchr(name, '=', end - name); + if (!eq) break; - *eq = '\0'; + value = eq + 1; + + /* The value has to be terminated within this area */ + nul = memchr(value, '\0', end - value); + if (!nul) + break; + + *eq = '\0'; nvram_set(h, name, value); *eq = '='; + + name = nul + 1; } /* Set special SDRAM parameters */ @@ -286,13 +306,13 @@ int nvram_commit(nvram_handle_t *h) header->config_ncdl = strtoul(ncdl, NULL, 0); } - /* Clear data area */ + /* Clear data area, only the mapped part belongs to this handle */ ptr = (char *) header + sizeof(nvram_header_t); - memset(ptr, 0xFF, nvram_part_size - h->offset - sizeof(nvram_header_t)); + memset(ptr, 0xFF, h->length - h->offset - sizeof(nvram_header_t)); memset(&tmp, 0, sizeof(nvram_header_t)); /* Leave space for a double NUL at the end */ - end = (char *) header + nvram_part_size - h->offset - 2; + end = (char *) header + h->length - h->offset - 2; /* Write out all tuples */ for (i = 0; i < NVRAM_ARRAYSIZE(h->nvram_hash); i++) { @@ -345,6 +365,8 @@ nvram_handle_t * nvram_open(const char *file, int rdonly) char *mtd = NULL; nvram_handle_t *h; nvram_header_t *header; + struct stat s; + size_t length, last; int offset = -1; /* If erase size or file are undefined then try to define them */ @@ -360,8 +382,31 @@ nvram_handle_t * nvram_open(const char *file, int rdonly) if( (fd = open(file ? file : mtd, O_RDWR)) > -1 ) { - char *mmap_area = (char *) mmap( - NULL, nvram_part_size, PROT_READ | PROT_WRITE, + char *mmap_area; + + /* + * A regular file, like the staging file, can be shorter than + * the nvram partition. Accessing the pages behind its end + * would raise SIGBUS, only map what the file provides. The + * size of a block device is not reported here, use the + * partition size for it. nvram_commit() pads the data to + * 4 bytes, keep the length a multiple of 4 for it. + */ + length = nvram_part_size; + if( fstat(fd, &s) > -1 && S_ISREG(s.st_mode) && + s.st_size < (off_t) length ) + length = (size_t) s.st_size & ~(size_t) 3; + + /* The header and the double NUL have to fit in */ + if( length < sizeof(nvram_header_t) + 4 ) + { + free(mtd); + close(fd); + return NULL; + } + + mmap_area = (char *) mmap( + NULL, length, PROT_READ | PROT_WRITE, (( rdonly == NVRAM_RO ) ? MAP_PRIVATE : MAP_SHARED) | MAP_LOCKED, fd, 0); if( mmap_area != MAP_FAILED ) @@ -369,9 +414,16 @@ nvram_handle_t * nvram_open(const char *file, int rdonly) /* * Start looking for NVRAM_MAGIC at beginning of MTD * partition. Stop if there is less than NVRAM_MIN_SPACE - * to check, that was the lowest used size. + * to check, that was the lowest used size. A smaller + * partition, like the one bcm47xxpart creates on a + * flash with 4 KiB erase blocks, can only have the + * NVRAM at its start. */ - for( i = 0; i <= ((nvram_part_size - NVRAM_MIN_SPACE) / sizeof(uint32_t)); i++ ) + last = 0; + if( length >= NVRAM_MIN_SPACE ) + last = (length - NVRAM_MIN_SPACE) / sizeof(uint32_t); + + for( i = 0; i <= last; i++ ) { if( ((uint32_t *)mmap_area)[i] == NVRAM_MAGIC ) { @@ -382,7 +434,7 @@ nvram_handle_t * nvram_open(const char *file, int rdonly) if( offset < 0 ) { - munmap(mmap_area, nvram_part_size); + munmap(mmap_area, length); free(mtd); close(fd); return NULL; @@ -392,7 +444,7 @@ nvram_handle_t * nvram_open(const char *file, int rdonly) { h->fd = fd; h->mmap = mmap_area; - h->length = nvram_part_size; + h->length = length; h->offset = offset; header = nvram_header(h);