Bug 207312 - Useless check in netipsec/key.c
Summary: Useless check in netipsec/key.c
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 10.2-STABLE
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-02-18 19:35 UTC by Mikhail Teterin
Modified: 2018-06-06 13:51 UTC (History)
4 users (show)

See Also:
dab: mfc-stable10+
dab: mfc-stable11-


Attachments
Remove a silly check, const-poison key-handling (14.20 KB, patch)
2016-02-18 19:35 UTC, Mikhail Teterin
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Teterin freebsd_committer 2016-02-18 19:35:20 UTC
Created attachment 167155 [details]
Remove a silly check, const-poison key-handling

The article at

  http://www.viva64.com/en/b/0377/

listed a problem with the KAME-derived code: the key_parse() function is comparing m->m_pkthdr.len with itself. We have this line since 2002, when sam committed what was than known as FAST_IPSEC option in base r105197.

The original KAME sources (https://github.com/kame/kame/) and NetBSD have this issue, but I could not find this code in OpenBSD cvs-repo online.

The minimal fix is to simply remove the useless check -- something the compiler must've been doing automatically ever since:

@@ -7245,9 +7245,8 @@ key_parse(struct mbuf *m, struct socket
        orglen = PFKEY_UNUNIT64(msg->sadb_msg_len);
        target = KEY_SENDUP_ONE;

-       if ((m->m_flags & M_PKTHDR) == 0 ||
-           m->m_pkthdr.len != m->m_pkthdr.len) {
-               ipseclog((LOG_DEBUG, "%s: invalid message length.\n",__func__));
+       if ((m->m_flags & M_PKTHDR) == 0) {
+               ipseclog((LOG_DEBUG, "%s: invalid message length.\n", __func__));
                PFKEYSTAT_INC(out_invlen);
                error = EINVAL;
                goto senderror;

However, the attached patch goes further and adds "const-poisoning" to functions in netipsec/key.c and netipsec/keysock.c . Please, review.
Comment 1 Andrey V. Elsukov freebsd_committer 2016-02-19 09:26:44 UTC
(In reply to Mikhail Teterin from comment #0)
> @@ -7245,9 +7245,8 @@ key_parse(struct mbuf *m, struct socket
>         orglen = PFKEY_UNUNIT64(msg->sadb_msg_len);
>         target = KEY_SENDUP_ONE;
> 
> -       if ((m->m_flags & M_PKTHDR) == 0 ||
> -           m->m_pkthdr.len != m->m_pkthdr.len) {
> -               ipseclog((LOG_DEBUG, "%s: invalid message
> length.\n",__func__));

The log message says about invalid length, probably, comparison should be done with orglen. I need some time to create the testing environment for this, can you test this by self?
Comment 2 Mikhail Teterin freebsd_committer 2016-02-19 15:56:35 UTC
(In reply to Andrey V. Elsukov from comment #1)
> can you test this by self?

No, sorry, I do not use IPSEC (for some reason)...
Comment 3 David Bright freebsd_committer 2018-03-09 18:58:15 UTC
It looks like this has been fixed in CURRENT; can this bug be closed now?
Comment 4 Mikhail T. 2018-03-09 20:05:19 UTC
After an MFC?
Comment 5 David Bright freebsd_committer 2018-03-09 20:51:07 UTC
It looks to me like this was fixed in 11 (and therefore CURRENT) by base r295967 and MFCed to stable/10 by base r296558.