Opened 5 years ago

Closed 4 years ago

#15436 closed defect (fixed)

Unaligned access in SipHash24 code

Reported by: pstumpf Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: 0.2.5.11
Severity: Keywords: crash 025-backport
Cc: rl1987@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Tor segfaults on OpenBSD/sparc64 on startup. GDB backtrace:

#0 siphash24 (src=0xfffffffffffcdc34, src_sz=20, key=Variable "key" is not avai
lable.
)

at src/ext/csiphash.c:118

118 memcpy(&mi, in, 8);
(gdb) bt
#0 siphash24 (src=0xfffffffffffcdc34, src_sz=20, key=Variable "key" is not avai
lable.
)

at src/ext/csiphash.c:118

#1 0x00000026f1b853c8 in node_get_mutable_by_id (identity_digest=Variable "iden
tity_digest" is not available.
)

As you can easily see, node_get_mutable_by_id passes an unaligned pointer to siphash24, which memcpy then tries to copy from. This is a (struct node_t)->identity, so maybe that struct should have alignment padding?

Child Tickets

Change History (8)

comment:1 Changed 5 years ago by rl1987

Cc: rl1987@… added

comment:2 in reply to:  description Changed 5 years ago by yawning

Status: newneeds_review

Replying to pstumpf:

As you can easily see, node_get_mutable_by_id passes an unaligned pointer to siphash24, which memcpy then tries to copy from. This is a (struct node_t)->identity, so maybe that struct should have alignment padding?

No, that's not the problem. The problem is const uint64_t *in = (uint64_t*)src;. The compiler is allowed to (and in your case) does assume that in is aligned properly.

Please let me know if this branch fixes this issue: https://github.com/Yawning/tor/compare/bug15436

comment:3 Changed 5 years ago by nickm

Keywords: crash added

This can go into 0.2.6 IMO, even though it isn't a regression.

comment:4 Changed 5 years ago by cypherpunks

Converting a pointer to void to a pointer of any object type doesn't require explicit casts.

comment:5 in reply to:  4 Changed 5 years ago by yawning

Replying to cypherpunks:

Converting a pointer to void to a pointer of any object type doesn't require explicit casts.

In C yeah. I was being consistent with the original code (and the upstream), which has the cast, presumably for the benefit of people building this with a C++ compiler.

Anyway, the submitter replied to the bug tracker e-mail confirming the branch works as advertised. If nickm cares about the cast, he can either tell me to change it, or change it when merging.

comment:6 Changed 5 years ago by nickm

Keywords: 025-backport added

comment:7 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.5.x-final

Cherry-picked to 0.2.5 as bug15436_025 in case we want to backport it to 0.2.5. Merged that to 0.2.6. Marking for possible backport.

comment:8 Changed 4 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final
Resolution: fixed
Status: needs_reviewclosed

Calling this "fixed in 0.2.6", wontfix on 0.2.5. Use 0.2.6 if you need the fix for this bug.

Note: See TracTickets for help on using tickets.