git: 255af72c8059 - main - netlink: Don't overwrite existing data in a linear buffer in snl_writer

The branch main has been updated by jhb:

URL: https://cgit.FreeBSD.org/src/commit/?id=255af72c8059b0117db646f82efa2e4848fa7570

commit 255af72c8059b0117db646f82efa2e4848fa7570
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2025-12-10 15:30:31 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2025-12-10 15:30:31 +0000

    netlink: Don't overwrite existing data in a linear buffer in snl_writer
    
    First, a bit of background on some of the data structures netlink uses
    to manage data associated with a netlink connection.
    
    - struct linear_buffer contains a single virtually-contiguous buffer
      of bytes.  Regions of this buffer are suballocated via lb_allocz()
      which uses a simple "bump" where the buffer is split into an
      allocated region at the start and a free region at the end.  Each
      allocation "bumps" the boundary (lb->offset) forward by the
      allocation size.
    
      Individual allocations are not freed.  Instead, the entire buffer is
      freed once all of the allocations are no longer in use.
    
      Linear buffers also contain an embedded link to permit chaining
      buffers together.
    
    - snl_state contains various state for a netlink connection including
      a chain of linear buffers.  This chain of linear buffers can contain
      allocations for netlink messages as well as other ancillary data
      buffers such as socket address structures.  The chain of linear
      buffers are freed once the connection is torn down.
    
    - snl_writer is used to construct a message written on a netlink
      connection.  It contains a single virtually-contiguous buffer
      (nw->base) allocated from the associated snl_state's linear buffer
      chain.  The buffer distinguishes between the amount of space
      reserved from the underlying allocator (nw->size) and the current
      message length actually written (nw->offset).  As new chunks of data
      (e.g. netlink attributes) are added to the write buffer, the buffer
      is grown by snl_realloc_msg_buffer by reallocating a larger buffer
      from the associated snl_state and copying over the current message
      data to the new buffer.
    
    Commit 0c511bafdd5b309505c13c8dc7c6816686d1e103 aimed to fix two bugs
    in snl_realloc_msg_buffer.
    
    The first bug is that snl_realloc_msg_buffer originally failed to
    update nw->size after growing the buffer which could result in
    spurious re-allocations when growing in the future.  It also probably
    could eventually lead to overflowing the buffer since each
    reallocation request was just adding the new bytes needed for a chunk
    to the original 'nw->size' while 'nw->offset' kept growing.
    Eventually the new 'nw->offset' would be larger than 'nw->size + sz'
    causing routines like snl_reserve_msg_data_raw() to return an
    out-of-bounds pointer.
    
    The second change in this commit I think was trying to fix the buffer
    overflows due to 'nw->size' being wrong, but instead introduced a new
    set of bugs.  The second change ignored the returned pointer from
    snl_allocz() and instead assumed it could use all of the
    currently-allocated data in the current linear buffer.  This is only
    ok if the only data in the linear buffer chain for the associated
    snl_state is the snl_writer's message buffer.  If there is any other
    data allocated from the snl_state, it could be earlier in the current
    linear buffer, so resetting new_base to nw->ss->lb->base can result in
    overwriting that other data.  The second change was also
    over-allocating storage from the underlying chain of linear buffers
    (e.g. a writer allocation of 256 followed by 512 would end up using
    the first 512 bytes, but 768 bytes would be reserved in the underlying
    linear buffer).
    
    To fix, revert the second change keeping only the fix for 'nw->size'
    being wrong.
    
    Reviewed by:    igoro, markj
    Fixes:          0c511bafdd5b ("netlink: fix snl_writer and linear_buffer re-allocation logic")
    Sponsored by:   AFRL, DARPA
    Differential Revision:  https://reviews.freebsd.org/D54148
---
 sys/netlink/netlink_snl.h | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/sys/netlink/netlink_snl.h b/sys/netlink/netlink_snl.h
index 57f7e1e29d08..52b35e502c98 100644
--- a/sys/netlink/netlink_snl.h
+++ b/sys/netlink/netlink_snl.h
@@ -1083,6 +1083,7 @@ static inline bool
 snl_realloc_msg_buffer(struct snl_writer *nw, size_t sz)
 {
 	uint32_t new_size = nw->size * 2;
+	char *new_base;
 
 	while (new_size < nw->size + sz)
 		new_size *= 2;
@@ -1090,23 +1091,20 @@ snl_realloc_msg_buffer(struct snl_writer *nw, size_t sz)
 	if (nw->error)
 		return (false);
 
-	if (snl_allocz(nw->ss, new_size) == NULL) {
+	new_base = snl_allocz(nw->ss, new_size);
+	if (new_base == NULL) {
 		nw->error = true;
 		return (false);
 	}
-	nw->size = new_size;
 
-	void *new_base = nw->ss->lb->base;
-	if (new_base != nw->base) {
-		memcpy(new_base, nw->base, nw->offset);
-		if (nw->hdr != NULL) {
-			int hdr_off = (char *)(nw->hdr) - nw->base;
+	memcpy(new_base, nw->base, nw->offset);
+	if (nw->hdr != NULL) {
+		int hdr_off = (char *)(nw->hdr) - nw->base;
 
-			nw->hdr = (struct nlmsghdr *)
-			    (void *)((char *)new_base + hdr_off);
-		}
-		nw->base = (char *)new_base;
+		nw->hdr = (struct nlmsghdr *)(void *)(new_base + hdr_off);
 	}
+	nw->base = new_base;
+	nw->size = new_size;
 
 	return (true);
 }