Page MenuHomeFreeBSD

nd6: Avoid using an uninitialized sockaddr in nd6_prefix_offlink()
ClosedPublic

Authored by markj on May 7 2021, 7:42 PM.

Details

Summary

Commit 81728a538 ("Split rtinit() into multiple functions.") removed
the initialization of sa6, but not one of its uses. This meant that we
were passing an uninitialized sockaddr as the address to
lltable_prefix_free(). Remove the variable outright to fix the problem.
The caller is expected to hold a reference on pr.

Reported by: KMSAN

Diff Detail

Repository
rG 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

markj requested review of this revision.May 7 2021, 7:42 PM

The free() is called outside of the interesting function an may be called on an still active component.

I'd suggest to handle the error != 0 case first an return in this branch early.
So the main search loop shifts to the main row and the code is simpler.

sys/netinet6/nd6_rtr.c
2189

If an error occured, a_failure is 1 and will never be reset.

The free() is called outside of the interesting function an may be called on an still active component.

Sorry, I'm having trouble understanding what you're suggesting here.

I'd suggest to handle the error != 0 case first an return in this branch early.
So the main search loop shifts to the main row and the code is simpler.

I can do that in a follow-up change. Here I would like to provide a minimal patch for the regression to simplify backporting and reduce the risk of another regression. In general I agree this code needs some general cleanup.

sys/netinet6/nd6_rtr.c
2189

Are you saying that we should not call lltable_prefix_free() in that case?

The free() is called outside of the interesting function an may be called on an still active component.

Sorry, I'm having trouble understanding what you're suggesting here.

My fault. I lost some characters while typing.

The code structure is:

allocate(prefix);
fail = 1;
if(good)
    forall()
       eventually_reset_fail;
else
    do_not_touch_fail;
if(fail)
    free(prefix);

There is more than one way through the code, where prefix is consumed or not.
My question is if all cases are fulfilled.

I'd suggest to handle the error != 0 case first an return in this branch early.
So the main search loop shifts to the main row and the code is simpler.

I can do that in a follow-up change. Here I would like to provide a minimal patch for the regression to simplify backporting and reduce the risk of another regression. In general I agree this code needs some general cleanup.

Okay. Sounds good.

This revision is now accepted and ready to land.May 11 2021, 4:42 PM

The free() is called outside of the interesting function an may be called on an still active component.

Sorry, I'm having trouble understanding what you're suggesting here.

My fault. I lost some characters while typing.

The code structure is:

allocate(prefix);
fail = 1;
if(good)
    forall()
       eventually_reset_fail;
else
    do_not_touch_fail;
if(fail)
    free(prefix);

There is more than one way through the code, where prefix is consumed or not.
My question is if all cases are fulfilled.

So I think the current behaviour is more or less intentional. Even if the route deletion fails, we want to purge the LLE, as the prefix isn't supposed to be used anymore. IPv4 behaves the same way.