Conversation
Signed-off-by: Joshua Rogers <[email protected]>
Signed-off-by: Joshua Rogers <[email protected]>
Signed-off-by: Joshua Rogers <[email protected]>
Sashan
left a comment
Sashan
left a comment
There was a problem hiding this comment.
please provide a core file. I just want to be sure this change has desired effect. thanks.
mattcaswell
left a comment
mattcaswell
left a comment
There was a problem hiding this comment.
I don't see the fix FreeBSD unaligned write promised by the title?
include/internal/ktls.h
Outdated
|
|
||
| ret = recvmsg(fd, &msg, 0); | ||
| if (ret < 0) | ||
| if (ret <= 0) |
There was a problem hiding this comment.
Why this change? A 0 return value from recvmsg means we successfully received a TLS record with 0 bytes in it. But that's ok. That's a success. We should probably let the upper layers figure out what to do with it.
There was a problem hiding this comment.
Does it? recv(2) states:
When a stream socket peer has performed an orderly shutdown, the return value will be 0 (the traditional "end-of-file" return).
and
Datagram sockets in various domains (e.g., the UNIX and Internet domains) permit zero-size datagrams. When such a datagram is received, the return value is 0.
kTLS runs over TCP, so the zero-sized datagram doesn't seem relevant here.
| /* returned length is limited to msg_iov.iov_len above */ | ||
| p[3] = (ret >> 8) & 0xff; | ||
| p[4] = ret & 0xff; | ||
| ret += prepend_length; |
There was a problem hiding this comment.
I wonder if we should just return immediately here and return -1 in the fall through case below. It really makes very little sense in returning success, where we have not prepended the record header. This would also make the memset that you added above redundant.
mattcaswell
added
branch: master
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
t8m
added
waiting-for: contributor response
|
ping @MegaManSec there are comments. |
MegaManSec
changed the title
Fixed (by removing it from the title and description). Originally I was going to submit this PR with #28860 combined, but decided to de-couple, but forgot to update the actual PR. |
mattcaswell
added
the
approval: review pending
t8m
added
triaged: refactor
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
- drop tag subtraction in recv buffer sizing - enforce MSG_EOR and reject MSG_CTRUNC - zero prepended header bytes before recvmsg Signed-off-by: Joshua Rogers <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #28861)
|
Squashed, applied clang-reformat and merged to the master branch. Thank you for your contribution. |
- drop tag subtraction in recv buffer sizing - enforce MSG_EOR and reject MSG_CTRUNC - zero prepended header bytes before recvmsg Signed-off-by: Joshua Rogers <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#28861)
5 participants
zero the 5 prepended TLS header bytes before recvmsg to avoid stale header bytes when no TLS_GET_RECORD_TYPE cmsg is delivered
require MSG_EOR and reject MSG_CTRUNC to avoid treating partial records as complete
drop subtraction of EVP_GCM_TLS_TAG_LEN from msg_iov.iov_len (kernel already strips the tag). Restores full plaintext capacity and matches the FreeBSD path