Opened 7 months ago

Closed 6 months ago

#24733 closed defect (fixed)

Loading ifc.ifc_buf using the new tor_free() causes undefined behaviour on x86_64 macOS

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: address-sanitizer, unexpected-consequences, review-group-28
Cc: Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: catalyst Sponsor: Sponsor8-can

Description

On macOS x86+64, the new tor_free() from #24337 loads ifc.ifc_buf, which leads to undefined behaviour. ifc.ifc_buf is a char * which should be aligned to a multiple 8 bytes, but it is always aligned at 8-bytes (ifc on the stack) plus 4 bytes (ifc_len and pragma pack(4)).

This bug was caused by #24337, which has been merged to master (0.3.3.0-alpha-dev), and Apple's 32/64 bit kernel data structure compatibility code.

It was discovered using our unit tests and clang's address sanitizer.

Child Tickets

Change History (16)

comment:1 Changed 7 months ago by teor

Actual Points: 0.1
Status: assignedneeds_review

Please see my branch bug24733.
I skipped the NULL check when calling raw_free(), because it's not required in C99.

comment:2 Changed 7 months ago by nickm

Status: needs_reviewmerge_ready

This patch looks fine to me.

I'm feeling dense here, but: what part *exactly* of the tor_free() macro is forbidden here? Is it the fact that we access the pointer via a char** ?

comment:3 Changed 7 months ago by teor

The newly introduced load to a local pointer variable from an address that's (N*8 + 4).
I don't think the type of the pointer matters.
Strangely, the comparison of that address to NULL, and the store of NULL to that address doesn't trigger the same warning in AddressSanitizer (or they are optimised out by the compiler!)

comment:4 Changed 7 months ago by catalyst

Shouldn't the #pragma pack(4) cause the compiler to generate the correct unaligned access fixup instructions for any C code that access that member? I would hope that clang's AddressSanitizer knows how to handle that exception. Oh wait, I see we copy the unaligned address of the pointer member to a normal (non-packed) local pointer variable. Yeah we should fix that.

We could also bypass many of these issues by fixing #24484.

comment:5 Changed 7 months ago by teor

Status: merge_readyneeds_review

catalyst and I talked about this at the patch party.

Instead of merging this branch, we think we should revert the evaluate-once change 1d348989b08a3d3c50dfa2600a5e5e2f61fb2b4a, to fix this issue.

Then we can open a ticket to decide if evaluate once is needed. (Unfortunately, we could not find any clang or gcc warnings about macro arguments with side-effects. Surely there has to be one?)

comment:6 Changed 7 months ago by nickm

I'm okay with reverting the evaluate-once change if we can replace it with something that prevents arguments with side-effects. Otherwise, I'm uncertain.

comment:7 Changed 7 months ago by nickm

Keywords: review-group-28 added

comment:8 Changed 7 months ago by catalyst

Another approach is making a special (static) free function that takes a pointer to struct ifconf and accesses ifc->ifc_buf directly. We could add comments there explaining why it's necessary (which also avoids cluttering the main body of get_interface_addresses_ioctl()).

comment:9 Changed 6 months ago by teor

Status: needs_reviewneeds_revision

comment:10 Changed 6 months ago by nickm

Let's figure out how to make progress here? Perhaps, merge this fix for now, and have a separate ticket for removing the evaluate-once code (if somebody wants to write the safety feature to prevent arguments with side-effects).

comment:11 in reply to:  8 ; Changed 6 months ago by teor

Replying to catalyst:

Another approach is making a special (static) free function that takes a pointer to struct ifconf and accesses ifc->ifc_buf directly. We could add comments there explaining why it's necessary (which also avoids cluttering the main body of get_interface_addresses_ioctl()).

I think refactoring my patch to do this in a separate function is a good solution.

And then a separate ticket for evaluate-once, if it is even possible.

comment:12 in reply to:  11 Changed 6 months ago by catalyst

Replying to teor:

I think refactoring my patch to do this in a separate function is a good solution.

And then a separate ticket for evaluate-once, if it is even possible.

Sounds good to me.

We might also want to comment the implementation of tor_free() to warn about this issue possibly arising elsewhere in the future when packed structures are involved.

comment:13 Changed 6 months ago by nickm

Status: needs_revisionneeds_review

I've tried to make the requested changes in branch bug24733. Okay to merge it?

comment:14 Changed 6 months ago by catalyst

Reviewer: catalyst

comment:15 Changed 6 months ago by catalyst

Status: needs_reviewmerge_ready

Thanks, looks good to me!

comment:16 Changed 6 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Thanks! Squashed and merging.

Note: See TracTickets for help on using tickets.