On 11/11/10 14:25, Jan Engelhardt wrote:
>
> On Thursday 2010-11-11 14:08, Pablo Neira Ayuso wrote:
>> On 11/11/10 00:08, Jan Engelhardt wrote:
>>> When nesting two mnl_attr_for_each loops, the __len__ variable will be
>>> declared twice, eliciting a warning when -Wshadow is turned on. There
>>> can also be warnings in pre-C99 because declarations and code are
>>> mixed. Do without any temporaries that are not explicitly specified as
>>> macro parameters.
>>
>> I like this spot, some question below:
>>
>>> -struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len)
>>> +struct nlattr *mnl_attr_next(const struct nlattr *attr)
>>> {
>>> - *len -= MNL_ALIGN(attr->nla_len);
>>> return (struct nlattr *)((void *)attr + MNL_ALIGN(attr->nla_len));
>>> }
>>
>> If we remove the len parameter from mnl_attr_next(), we may access
>> memory that may be out of the message boundary in mnl_attr_ok().
>
> Not that I can see; mnl_attr_ok tests for len >= sizeof(struct nlattr),
> and len is tail minus attr.
mnl_attr_for_each() in your patch is OK, sorry. But, here:
+#define mnl_attr_for_each_nested(attr, nest) \
+ for ((attr) = mnl_attr_get_payload(nest); \
+ mnl_attr_ok((attr), mnl_attr_get_payload(attr) +
mnl_attr_get_payload_len(attr) - (void *)(attr)); \
+ (attr) = mnl_attr_next(attr))
Once we iterate over the last attribute in the nest, we iterate again to
check if there's any next. Then, mnl_attr_get_payload may access
attr->len for an attribute that doesn't belong the nest or (if the nest
is in the end of the message) an out of bound message access.
I think that we can add mnl_attr_get_payload_tail to make tail minus
attr, like in mnl_attr_for_each().
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
