Hi Marcelo,
On Wed, Oct 29, 2014 at 10:51:14AM -0200, Marcelo Ricardo Leitner wrote:
> Currently, we don't check if __build_packet_message fails or not and
> that leaves it prone to sending incomplete messages to userspace.
>
> This commit fixes it by canceling the new message if there was any issue
> while building it and makes sure the skb is freed if it was the only
> message in it.
>
> Signed-off-by: Marcelo Ricardo Leitner <mleitner@xxxxxxxxxx>
> ---
> net/netfilter/nfnetlink_log.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> index b1e3a05794169283ed50d1c0fb4f44d9e7753eeb..045c5956611f61a3f5dad3e15ca7cf19365e5afa 100644
> --- a/net/netfilter/nfnetlink_log.c
> +++ b/net/netfilter/nfnetlink_log.c
> @@ -568,10 +568,8 @@ __build_packet_message(struct nfnl_log_net *log,
> struct nlattr *nla;
> int size = nla_attr_size(data_len);
>
> - if (skb_tailroom(inst->skb) < nla_total_size(data_len)) {
> - printk(KERN_WARNING "nfnetlink_log: no tailroom!\n");
> - return -1;
> - }
> + if (skb_tailroom(inst->skb) < nla_total_size(data_len))
> + goto nla_put_failure;
>
> nla = (struct nlattr *)skb_put(inst->skb, nla_total_size(data_len));
> nla->nla_type = NFULA_PAYLOAD;
> @@ -586,6 +584,7 @@ __build_packet_message(struct nfnl_log_net *log,
>
> nla_put_failure:
> PRINTR(KERN_ERR "nfnetlink_log: error creating log nlmsg\n");
> + nlmsg_cancel(skb, nlh);
> return -1;
> }
>
> @@ -708,8 +707,9 @@ nfulnl_log_packet(struct net *net,
>
> inst->qlen++;
>
> - __build_packet_message(log, inst, skb, data_len, pf,
> - hooknum, in, out, prefix, plen);
> + if (__build_packet_message(log, inst, skb, data_len, pf,
> + hooknum, in, out, prefix, plen))
> + goto build_failure;
>
> if (inst->qlen >= qthreshold)
> __nfulnl_flush(inst);
> @@ -726,6 +726,15 @@ unlock_and_release:
> instance_put(inst);
> return;
>
> +build_failure:
> + /* If no other messages in it, we're good to free it. */
> + if (!inst->skb->len) {
> + kfree_skb(inst->skb);
> + inst->skb = NULL;
> + }
> +
> + inst->qlen--;
For each message that we put into the batch, we increase inst->qlen in
one, so I think this decrement isn't enough to leave things in
consistent state. If we at least have one message already in the
batch, I think it's good to give it a try to deliver it to userspace:
if (inst->skb)
__nfulnl_flush(inst);
Then, the WARN_ON that Florian added recently should catch that we
have size miscalculations from __nfulnl_send().
--
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
