Re: svn commit: r265691 - head/sys/netinet
On Thu, May 08, 2014 at 05:27:46PM +, Michael Tuexen wrote:
M Author: tuexen
M Date: Thu May 8 17:27:46 2014
M New Revision: 265691
M URL: http://svnweb.freebsd.org/changeset/base/265691
M
M Log:
M For some UDP packets (for example with 200 byte payload) and IP options,
M the IP header and the UDP header are not in the same mbuf.
M Add code to in_delayed_cksum() to deal with this case.
M
M MFC after: 3 days
M
M Modified:
M head/sys/netinet/ip_output.c
M
M Modified: head/sys/netinet/ip_output.c
M
==
M --- head/sys/netinet/ip_output.c Thu May 8 17:20:45 2014
(r265690)
M +++ head/sys/netinet/ip_output.c Thu May 8 17:27:46 2014
(r265691)
M @@ -887,15 +887,23 @@ in_delayed_cksum(struct mbuf *m)
M csum = 0x;
M offset += m-m_pkthdr.csum_data;/* checksum offset */
M
M +/* find the mbuf in the chain where the checksum starts*/
M +while ((m != NULL) (offset = m-m_len)) {
M +offset -= m-m_len;
M +m = m-m_next;
M +}
M +if (m == NULL) {
M +/* This should not happen. */
M +printf(in_delayed_cksum(): checksum outside mbuf chain.\n);
M +return;
M +}
M if (offset + sizeof(u_short) m-m_len) {
M -printf(delayed m_pullup, m-len: %d off: %d p: %d\n,
M -m-m_len, offset, ip-ip_p);
M /*
M * XXX
M - * this shouldn't happen, but if it does, the
M - * correct behavior may be to insert the checksum
M - * in the appropriate next mbuf in the chain.
M + * This should not happen, but if it does, it might make more
M + * sense to fix the caller than to add code to split it here.
M */
M +printf(in_delayed_cksum(): checksum split between mbufs.\n);
M return;
M }
M *(u_short *)(m-m_data + offset) = csum;
If this should not happen really means that we do not expect this issue
at all, assuming we are coding correctly, then all these comments and printfs
should be converted to KASSERTs.
If this should not happen means 'such packets shouldn't arrrive from
network, then we need to remove printfs, because having a remotely
triggerable printf(9) is a DDoS vulnerability.
--
Totus tuus, Glebius.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r265691 - head/sys/netinet
On 08 May 2014, at 22:23, Gleb Smirnoff gleb...@freebsd.org wrote:
On Thu, May 08, 2014 at 05:27:46PM +, Michael Tuexen wrote:
M Author: tuexen
M Date: Thu May 8 17:27:46 2014
M New Revision: 265691
M URL: http://svnweb.freebsd.org/changeset/base/265691
M
M Log:
M For some UDP packets (for example with 200 byte payload) and IP options,
M the IP header and the UDP header are not in the same mbuf.
M Add code to in_delayed_cksum() to deal with this case.
M
M MFC after: 3 days
M
M Modified:
M head/sys/netinet/ip_output.c
M
M Modified: head/sys/netinet/ip_output.c
M
==
M --- head/sys/netinet/ip_output.c Thu May 8 17:20:45 2014
(r265690)
M +++ head/sys/netinet/ip_output.c Thu May 8 17:27:46 2014
(r265691)
M @@ -887,15 +887,23 @@ in_delayed_cksum(struct mbuf *m)
Mcsum = 0x;
Moffset += m-m_pkthdr.csum_data;/* checksum offset */
M
M + /* find the mbuf in the chain where the checksum starts*/
M + while ((m != NULL) (offset = m-m_len)) {
M + offset -= m-m_len;
M + m = m-m_next;
M + }
M + if (m == NULL) {
M + /* This should not happen. */
M + printf(in_delayed_cksum(): checksum outside mbuf chain.\n);
M + return;
M + }
Mif (offset + sizeof(u_short) m-m_len) {
M - printf(delayed m_pullup, m-len: %d off: %d p: %d\n,
M - m-m_len, offset, ip-ip_p);
M/*
M * XXX
M - * this shouldn't happen, but if it does, the
M - * correct behavior may be to insert the checksum
M - * in the appropriate next mbuf in the chain.
M + * This should not happen, but if it does, it might make more
M + * sense to fix the caller than to add code to split it here.
M */
M + printf(in_delayed_cksum(): checksum split between mbufs.\n);
Mreturn;
M}
M*(u_short *)(m-m_data + offset) = csum;
If this should not happen really means that we do not expect this issue
at all, assuming we are coding correctly, then all these comments and printfs
should be converted to KASSERTs.
I tried to keep the style of the code...
However, if the first one occurs, we are setting up a packet and have it too
short to contain the checksum. This would really be bug in our code... So a
KASSERT is fine.
A KASSERT for the second case is also fine.
The only difference between the above and the KASSERT version is that
in case of problems the above just sends packets with wrong checksums
and the KASSERT version will panic (when compiled with INVARIANTS) or
panic (in case of m == NULL) or corrupt a byte. However, I also prefer
the KASSERT version, it will help to get the code right...
So I committed a change in
http://svnweb.freebsd.org/changeset/base/265713
Thanks for your suggestion!
Best regards
Michael
If this should not happen means 'such packets shouldn't arrrive from
network, then we need to remove printfs, because having a remotely
triggerable printf(9) is a DDoS vulnerability.
--
Totus tuus, Glebius.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r265691 - head/sys/netinet
On Thu, May 08, 2014 at 10:50:28PM +0200, Michael Tuexen wrote: M If this should not happen really means that we do not expect this issue M at all, assuming we are coding correctly, then all these comments and printfs M should be converted to KASSERTs. M I tried to keep the style of the code... M However, if the first one occurs, we are setting up a packet and have it too M short to contain the checksum. This would really be bug in our code... So a KASSERT is fine. M A KASSERT for the second case is also fine. M The only difference between the above and the KASSERT version is that M in case of problems the above just sends packets with wrong checksums M and the KASSERT version will panic (when compiled with INVARIANTS) or M panic (in case of m == NULL) or corrupt a byte. However, I also prefer M the KASSERT version, it will help to get the code right... M So I committed a change in M http://svnweb.freebsd.org/changeset/base/265713 M M Thanks for your suggestion! Thanks a lot, Michael! -- Totus tuus, Glebius. ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
