On Mon, Sep 11, 2023 at 09:21:50PM +0200, Pablo Neira Ayuso wrote: > On Sun, Sep 10, 2023 at 09:30:18PM +0100, Jeremy Sowden wrote: > > `mnl_nlmsg_ok` and `mnl_attr_ok` both expect a signed buffer length > > value, `len`, against which to compare the size of the object expected > > to fit into the buffer, because they are intended to validate the length > > and it may be negative in the case of malformed messages. Comparing > > this signed value against unsigned operands leads to compiler warnings, > > so the unsigned operands are cast to `int`. Comparing `len` to the size > > of the structure is fine, because the structures are only a few bytes in > > size. Comparing it to the length fields of `struct nlmsg` and `struct > > nlattr`, however, is problematic, since these fields may hold values > > greater than `INT_MAX`, in which case the casts will yield negative > > values and result in false positives. > > > > Instead, assign `len` to an unsigned local variable, check for negative > > values first, then use the unsigned local for the other comparisons, and > > remove the casts. > > > > Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1691 > > Signed-off-by: Jeremy Sowden <jeremy@xxxxxxxxxx> > > --- > > src/attr.c | 9 +++++++-- > > src/nlmsg.c | 9 +++++++-- > > 2 files changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/src/attr.c b/src/attr.c > > index bc39df4199e7..48e95019d5e8 100644 > > --- a/src/attr.c > > +++ b/src/attr.c > > @@ -97,9 +97,14 @@ EXPORT_SYMBOL void *mnl_attr_get_payload(const struct nlattr *attr) > > */ > > EXPORT_SYMBOL bool mnl_attr_ok(const struct nlattr *attr, int len) > > Maybe turn this into uint32_t ? Actually, attribute length field is 16 bits long, so it can never happen that nla_len will underflow.
