Page MenuHomeFreeBSD

netgraph/ng_checksum: Fix double free error
ClosedPublic

Authored by donner on May 15 2021, 9:38 AM.

Details

Summary

m_pullup(9) frees the mbuf(9) chain in the case of an allocation error.
The mbuf chain must not be freed again at the end in this case.

PR: 255874
Submitted by: <lylgood@foxmail.com>
MFC after: 1 week

Test Plan

Tested by the submitter.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

checksum_ipv4() and checksum_ipv6() may fail without freeing the mbuf. In that case, with the patch the mbuf is leaked.

One approach that's used in some places is to pass a struct mbuf ** to the subroutine, and have the callee set *m = NULL when it frees the mbuf. Alternately, checksum_ip*() can be modified to always free the mbuf upon failure.

sys/netgraph/ng_checksum.c
686

drop is not really an accurate name for the label now.

checksum_ipv4() and checksum_ipv6() may fail without freeing the mbuf. In that case, with the patch the mbuf is leaked.

Can you point to a code line, where this can happen?
I didn't see one.

One approach that's used in some places is to pass a struct mbuf ** to the subroutine,
and have the callee set *m = NULL when it frees the mbuf. Alternately, checksum_ip*()
can be modified to always free the mbuf upon failure.

As part of a brushup of this code, yes.
As part of a bug fix, no.

sys/netgraph/ng_checksum.c
686

Not as part of a bugfix.

checksum_ipv4() and checksum_ipv6() may fail without freeing the mbuf. In that case, with the patch the mbuf is leaked.

Can you point to a code line, where this can happen?
I didn't see one.

In checksum_ipv4() we call PULLUP_CHECK() on line 316. PULLUP_CHECK() looks like this:

293 #define PULLUP_CHECK(mbuf, length) do {                                 \                                                                                                 
294         pullup_len += length;                                           \                                                                                                 
295         if (((mbuf)->m_pkthdr.len < pullup_len) ||                      \                                                                                                 
296             (pullup_len > MHLEN)) {                                     \                                                                                                 
297                 return (EINVAL);                                        \                                                                                                 
298         }                                                               \                                                                                                 
299         if ((mbuf)->m_len < pullup_len &&                               \                                                                                                 
300             (((mbuf) = m_pullup((mbuf), pullup_len)) == NULL)) {        \                                                                                                 
301                 return (ENOBUFS);                                       \                                                                                                 
302         }                                                               \                                                                                                 
303 } while (0)

On line 297, we return an error without having freed the mbuf. On line 301 we return an error having freed the mbuf, and the caller of checksum_ipv4() will free the mbuf again. So in the first case, we are now not freeing the mbuf at all.

One approach that's used in some places is to pass a struct mbuf ** to the subroutine,
and have the callee set *m = NULL when it frees the mbuf. Alternately, checksum_ip*()
can be modified to always free the mbuf upon failure.

As part of a brushup of this code, yes.
As part of a bug fix, no.

To be clear, what I am proposing there is part of the bug fix, not a cleanup.

checksum_ipv4() and checksum_ipv6() may fail without freeing the mbuf. In that case, with the patch the mbuf is leaked.

Can you point to a code line, where this can happen?
I didn't see one.

In checksum_ipv4() we call PULLUP_CHECK() on line 316. PULLUP_CHECK() looks like this:

293 #define PULLUP_CHECK(mbuf, length) do {                                 \                                                                                                 
294         pullup_len += length;                                           \                                                                                                 
295         if (((mbuf)->m_pkthdr.len < pullup_len) ||                      \                                                                                                 
296             (pullup_len > MHLEN)) {                                     \                                                                                                 
297                 return (EINVAL);                                        \                                                                                                 
298         }                                                               \                                                                                                 
299         if ((mbuf)->m_len < pullup_len &&                               \                                                                                                 
300             (((mbuf) = m_pullup((mbuf), pullup_len)) == NULL)) {        \                                                                                                 
301                 return (ENOBUFS);                                       \                                                                                                 
302         }                                                               \                                                                                                 
303 } while (0)

On line 297, we return an error without having freed the mbuf. On line 301 we return an error having freed the mbuf, and the caller of checksum_ipv4() will free the mbuf again. So in the first case, we are now not freeing the mbuf at all.

This is well respected on the caller side:

	error = checksum_ipv*(priv, m, pullup_len);
	if (error == 0)
		goto bypass;
	else if (error == ENOBUFS)
		goto drop;

I don't say, the bug fix could be improved, ...

checksum_ipv4() and checksum_ipv6() may fail without freeing the mbuf. In that case, with the patch the mbuf is leaked.

Can you point to a code line, where this can happen?
I didn't see one.

In checksum_ipv4() we call PULLUP_CHECK() on line 316. PULLUP_CHECK() looks like this:

293 #define PULLUP_CHECK(mbuf, length) do {                                 \                                                                                                 
294         pullup_len += length;                                           \                                                                                                 
295         if (((mbuf)->m_pkthdr.len < pullup_len) ||                      \                                                                                                 
296             (pullup_len > MHLEN)) {                                     \                                                                                                 
297                 return (EINVAL);                                        \                                                                                                 
298         }                                                               \                                                                                                 
299         if ((mbuf)->m_len < pullup_len &&                               \                                                                                                 
300             (((mbuf) = m_pullup((mbuf), pullup_len)) == NULL)) {        \                                                                                                 
301                 return (ENOBUFS);                                       \                                                                                                 
302         }                                                               \                                                                                                 
303 } while (0)

On line 297, we return an error without having freed the mbuf. On line 301 we return an error having freed the mbuf, and the caller of checksum_ipv4() will free the mbuf again. So in the first case, we are now not freeing the mbuf at all.

This is well respected on the caller side:

	error = checksum_ipv*(priv, m, pullup_len);
	if (error == 0)
		goto bypass;
	else if (error == ENOBUFS)
		goto drop;

I don't say, the bug fix could be improved, ...

Sorry, I missed this detail when reading the code. I think the diff is fine.

This revision is now accepted and ready to land.May 16 2021, 5:01 PM
This revision was automatically updated to reflect the committed changes.