On Sun, Jun 12, 2016 at 11:40:57PM -0400, Vishwanath Pai wrote: > On 06/09/2016 01:57 PM, Vishwanath Pai wrote: > > On 06/08/2016 08:16 AM, Pablo Neira Ayuso wrote: > >> Looking again at your code: > >> > >> case NFULNL_COPY_PACKET: > >> - if (inst->copy_range > skb->len) > >> + data_len = inst->copy_range; > >> + if (li->u.ulog.copy_len < data_len) > >> + data_len = li->u.ulog.copy_len; > >> > >> data_len is set to instance's copy_range. > >> > >> But then, if the NFLOG rule indicates smaller copy_len, you use this > >> value. So to my understanding, NFLOG rule prevails over instance's > >> copy_range in what matters, which is to shrink the copy range. > >> > >>>> --nflog-range will not override the per-instance default, > >>>> the only time it would get preference is when its value is lesser than > >>>> the per-instance value. If copy_range is lesser than --nflog-range then > >>>> we retain copy_range. > >>>> > >>>> So basically what we are doing is min(copy_range, nflog-range). > >>>> Just wanted to clarify this, if this is not how it's meant to be > >>>> please let me know. > >>>> > >>>> Also, there is a bug in my patch, li->u.ulog.copy_len can be set to "0" > >>>> from userspace (if --nflog-range is not specified), so we have to check > >>>> for this condition before using the value. I will send a V2 of the patch > >>>> based on your reply. > >> Currently, li->u.ulog.copy_len is set to "0" by default when not > >> specified. > >> > >> But copy_len = 0 is a valid possibility, so this looks a bit more > >> tricky to me to fix since we may need to get flags here to know when > >> this is set. > >> > >> Probably something like: > >> > >> if (li->flags & NF_LOG_F_COPY_RANGE) > >> data_len = li->u.ulog.copy_len; > >> /* Per-instance copy range prevails over global per-rule option. */ > >> if (data_len < inst->copy_range) > >> data_len = inst->copy_range; > >> if (data_len > skb->len) > >> data_len = skb->len; > >> > >> Although this would require a bit more code to introduce these flags. > >> > >> You will also need a new flag for xt_NFLOG: > >> > >> #define XT_NFLOG_COPY_LEN 0x2 > >> > >> it seems other XT_NFLOG_* flags were already in place. > >> > >> Interesting that nobody noticed this for so long BTW. > > > > I tried this out, I added two flags: one for XT_NFLOG to notify the > > kernel when --nflog-range is set by the user, and another flag for > > nfnetlink_log to pass this on to nfulnl_log_packet. This design works > > fine but while testing this I found a problem. > > > > Lets say --nflog-range is set to 200, and the instance copy_range is set > > to 100. According to the code above the final value of data_len will be > > 200 so we try to pack 200 bytes into the skb. But somewhere between > > nfnetlink_log to dumpcap the packet is getting truncated and dumpcap > > doesn't like this: > > > > $> dumpcap -y NFLOG -i nflog:5 -s 100 > > Capturing on 'nflog:5' > > File: /tmp/wireshark_pcapng_nflog-5_20160609133531_pi6MrS > > dumpcap: Error while capturing packets: Message truncated: (got: 228) > > (nlmsg_len: 320) > > Please report this to the Wireshark developers. > > https://bugs.wireshark.org/ > > (This is not a crash; please do not report it as such.) > > Packets captured: 0 > > Packets received/dropped on interface 'nflog:5': 0/0 > > (pcap:0/dumpcap:0/flushed:0/ps_ifdrop:0) (0.0%) > > > > I'm trying to figure out where the packet is getting truncated. > > > > I found where the problem is. This is the userspace code for libpcap: > > do { > len = recv(handle->fd, handle->buffer, handle->bufsize, 0); > if (handle->break_loop) { > handle->break_loop = 0; > return -2; > } > } while ((len == -1) && (errno == EINTR)); > > ... > > buf = handle->buffer; > while (len >= NLMSG_SPACE(0)) { > const struct nlmsghdr *nlh = (const struct nlmsghdr *) buf; > u_int32_t msg_len; > nftype_t type = OTHER; > > if (nlh->nlmsg_len < sizeof(struct nlmsghdr) || len < > nlh->nlmsg_len) { > snprintf(handle->errbuf, PCAP_ERRBUF_SIZE, > "Message truncated: (got: %d) (nlmsg_len: %u)", len, nlh->nlmsg_len); > return -1; > } > > handle->bufsize is only big enough to accommodate the snaplen specified > by the user in dumpcap. So if we send more data than that then we will > break userspace. One way around this is to send min(li->u.ulog.copy_len, > inst->copy_range). If this is OK then I can send a patch for this, > please suggest. But nlmsg_len should match len in this. If we're just sending a part of the packet to userspace, then we should adjust nlmsg_len to indicate exactly the netlink message length that we're sending to userspace. Is your patch triggering this nlmsg_len != len? -- 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
