Re: [PATCH v2 1/4] packet: replace struct desc by struct iovec

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 15 Jul 2024 14:59:42 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jul 12, 2024 at 05:32:41PM +0200, Laurent Vivier wrote:
> > To be able to manage buffers inside a shared memory provided
> > by a VM via a vhost-user interface, we cannot rely on the fact
> > that buffers are located in a pre-defined memory area and use
> > a base address and a 32bit offset to address them.
> > 
> > We need a 64bit address, so replace struct desc by struct iovec
> > and update range checking.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >  packet.c | 84 +++++++++++++++++++++++++++++++-------------------------
> >  packet.h | 14 ++--------
> >  2 files changed, 49 insertions(+), 49 deletions(-)
> > 
> > diff --git a/packet.c b/packet.c
> > index ccfc84607709..f7bb523c4ffa 100644
> > --- a/packet.c
> > +++ b/packet.c
> > @@ -22,6 +22,39 @@
> >  #include "util.h"
> >  #include "log.h"
> >  
> > +/**
> > + * packet_check_range() - Check if a packet memory range is valid
> > + * @p:		Packet pool
> > + * @offset:	Offset of data range in packet descriptor
> > + * @len:	Length of desired data range
> > + * @start:	Start of the packet descriptor
> > + * @func:	For tracing: name of calling function, NULL means no trace()
> > + * @line:	For tracing: caller line of function call
> > + *
> > + * Return: 0 if the range is valid, -1 otherwise
> > + */
> > +static int packet_check_range(const struct pool *p, size_t offset, size_t len,
> > +			      const char *start, const char *func, int line)
> > +{
> > +	if (start < p->buf) {
> > +		if (func) {  
> 
> Omitting the message entirely if func is not set doesn't seem correct.
> I believe printf() should format NULL pointers sanely (typically as
> "<null>"), so I think you can just leave out this check.

That intention is actually pre-existing: look at the function comment
(coming from packet_add_do()).

Originally, I wanted to implement --trace like that: if no function
name was given, no messages would be printed. Then I realised it wasn't
really practical and changed to a static logging flag, but I still
accidentally left this in commit bb708111833e ("treewide: Packet
abstraction with mandatory boundary checks").

Anyway, yes, func is always passed, so there's no need for this check
(and sure, there would be no _need_ anyway). We just need to fix the
function comments.

> > +			trace("add packet start %p before buffer start %p, "

It's not "add" if it's called from packet_get_do(). As we print the
function name anyway, we could drop "add " from this altogether, it
should be clear enough.

> > +			      "%s:%i", (void *)start, (void *)p->buf, func, line);
> > +		}
> > +		return -1;
> > +	}
> > +
> > +	if (start + len + offset > p->buf + p->buf_size) {  
> 
> It's not really clear to me why offset is needed in here.  AIUI,
> offset is used when we want to talk about some piece of a larger
> packet/frame that's in the buffer.  That's useful when we're
> dissecting packets,

...and that's packet_get_do()'s usage, passing a non-zero offset here
(stricter check anyway), while:

> but surely we always want the whole frame/whatever
> to be within the buffer,

packet_add_do() calls this with a zero offset, because the whole packet
should fit.

That is, this check replaces:

	if (start + len > p->buf + p->buf_size) {

from packet_add_do(), and:

	if (p->pkt[idx].offset + len + offset > p->buf_size) {

from packet_get_do(). It looks equivalent to me.

> so I don't know we need the extra complexity
> in this helper.
> 
> I also think we should check for overflow on the LHS here, but that's
> pre-existing, so it doesn't need to go in this patch.
> 
> > +		if (func) {
> > +			trace("packet offset plus length %lu from size %lu, "
> > +			      "%s:%i", start - p->buf + len + offset,
> > +			      p->buf_size, func, line);
> > +		}
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> >  /**
> >   * packet_add_do() - Add data as packet descriptor to given pool
> >   * @p:		Existing pool
> > @@ -41,34 +74,16 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
> >  		return;
> >  	}
> >  
> > -	if (start < p->buf) {
> > -		trace("add packet start %p before buffer start %p, %s:%i",
> > -		      (void *)start, (void *)p->buf, func, line);
> > +	if (packet_check_range(p, 0, len, start, func, line))
> >  		return;
> > -	}
> > -
> > -	if (start + len > p->buf + p->buf_size) {
> > -		trace("add packet start %p, length: %zu, buffer end %p, %s:%i",
> > -		      (void *)start, len, (void *)(p->buf + p->buf_size),
> > -		      func, line);
> > -		return;
> > -	}
> >  
> >  	if (len > UINT16_MAX) {
> >  		trace("add packet length %zu, %s:%i", len, func, line);
> >  		return;
> >  	}
> >  
> > -#if UINTPTR_MAX == UINT64_MAX
> > -	if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) {
> > -		trace("add packet start %p, buffer start %p, %s:%i",
> > -		      (void *)start, (void *)p->buf, func, line);
> > -		return;
> > -	}
> > -#endif
> > -
> > -	p->pkt[idx].offset = start - p->buf;
> > -	p->pkt[idx].len = len;
> > +	p->pkt[idx].iov_base = (void *)start;
> > +	p->pkt[idx].iov_len = len;
> >  
> >  	p->count++;
> >  }
> > @@ -96,36 +111,31 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
> >  		return NULL;
> >  	}
> >  
> > -	if (len > UINT16_MAX || len + offset > UINT32_MAX) {
> > +	if (len > UINT16_MAX) {
> >  		if (func) {
> > -			trace("packet data length %zu, offset %zu, %s:%i",
> > -			      len, offset, func, line);
> > +			trace("packet data length %zu, %s:%i",
> > +			      len, func, line);  
> 
> Should this be an assert?  Seems like something is wrong in the
> caller, if they're trying to pass in a ludicrously long packet.

Maybe something is wrong in the caller, but these are sanity checks for
security's sake, so if somebody finds out how to reach here with a
ludicrously long packet, I think it's preferable to discard the packet
rather than crashing and turning whatever issue into a vulnerability.

> >  		}
> >  		return NULL;
> >  	}
> >  
> > -	if (p->pkt[idx].offset + len + offset > p->buf_size) {
> > +	if (len + offset > p->pkt[idx].iov_len) {
> >  		if (func) {
> > -			trace("packet offset plus length %zu from size %zu, "
> > -			      "%s:%i", p->pkt[idx].offset + len + offset,
> > -			      p->buf_size, func, line);
> > +			trace("data length %zu, offset %zu from length %zu, "
> > +			      "%s:%i", len, offset, p->pkt[idx].iov_len,
> > +			      func, line);
> >  		}
> >  		return NULL;
> >  	}
> >  
> > -	if (len + offset > p->pkt[idx].len) {
> > -		if (func) {
> > -			trace("data length %zu, offset %zu from length %u, "
> > -			      "%s:%i", len, offset, p->pkt[idx].len,
> > -			      func, line);
> > -		}
> > +	if (packet_check_range(p, offset, len, p->pkt[idx].iov_base,
> > +			       func, line))
> >  		return NULL;
> > -	}
> >  
> >  	if (left)
> > -		*left = p->pkt[idx].len - offset - len;
> > +		*left = p->pkt[idx].iov_len - offset - len;
> >  
> > -	return p->buf + p->pkt[idx].offset + offset;
> > +	return (char *)p->pkt[idx].iov_base + offset;
> >  }
> >  
> >  /**
> > diff --git a/packet.h b/packet.h
> > index a784b07bbed5..8377dcf678bb 100644
> > --- a/packet.h
> > +++ b/packet.h
> > @@ -6,16 +6,6 @@
> >  #ifndef PACKET_H
> >  #define PACKET_H
> >  
> > -/**
> > - * struct desc - Generic offset-based descriptor within buffer
> > - * @offset:	Offset of descriptor relative to buffer start, 32-bit limit
> > - * @len:	Length of descriptor, host order, 16-bit limit
> > - */
> > -struct desc {
> > -	uint32_t offset;
> > -	uint16_t len;
> > -};
> > -
> >  /**
> >   * struct pool - Generic pool of packets stored in a buffer
> >   * @buf:	Buffer storing packet descriptors
> > @@ -29,7 +19,7 @@ struct pool {
> >  	size_t buf_size;
> >  	size_t size;
> >  	size_t count;
> > -	struct desc pkt[1];
> > +	struct iovec pkt[1];
> >  };
> >  
> >  void packet_add_do(struct pool *p, size_t len, const char *start,
> > @@ -54,7 +44,7 @@ struct _name ## _t {							\
> >  	size_t buf_size;						\
> >  	size_t size;							\
> >  	size_t count;							\
> > -	struct desc pkt[_size];						\
> > +	struct iovec pkt[_size];					\
> >  }
> >  
> >  #define PACKET_POOL_INIT_NOCAST(_size, _buf, _buf_size)			\  

-- 
Stefano