[PATCH 0/4] MR384: winsock: Support fallback to ICMP over SOCK_DGRAM on Linux
ICMP over SOCK_DGRAM is already used in ndispproxy.sys. But apps may also use ICMP directly over SOCK_RAW (Hardspace: Shipbreaker is an example) and that requires admin privileges on Linux (or setcap cap_net_raw+ep on wineserver). AF_INET / SOCK_RAW / IPPROTO_ICMP works on Windows without admin privileges. Comments: - IP, ICMP header structures and chksum() functions are taken from ndisproxy.sys:icmp_echo.c; - ICMP over DGRAM, while designed specifically to allow ICMP without admin permissions, still needs some setup Linux, /proc/sys/net/ipv4/ping_group_range controls that. Such socket creation fails on Testbot Debian machines so I left skip() path in test_icmp(); - Linux substitute local port number instead of provided ICMP packet id, while the referenced game depends on reply id matching the id it provided in request. So the last part performs the fixup. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/384
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/socket.c | 30 ++++++++++++++++++++++++------ dlls/ws2_32/tests/protocol.c | 2 ++ dlls/ws2_32/tests/sock.c | 23 +++++++++++++++++++++-- 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 9cc75aacf20..d3c93e15a47 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -70,6 +70,23 @@ static const WSAPROTOCOL_INFOW supported_protocols[] = .dwMessageSize = 0xffbb, .szProtocol = L"UDP/IP", }, + { + .dwServiceFlags1 = XP1_IFS_HANDLES | XP1_SUPPORT_BROADCAST + | XP1_SUPPORT_MULTIPOINT | XP1_MESSAGE_ORIENTED | XP1_CONNECTIONLESS, + .dwProviderFlags = PFL_MATCHES_PROTOCOL_ZERO | PFL_HIDDEN, + .ProviderId = {0xe70f1aa0, 0xab8b, 0x11cf, {0x8c, 0xa3, 0x00, 0x80, 0x5f, 0x48, 0xa1, 0x92}}, + .dwCatalogEntryId = 1003, + .ProtocolChain.ChainLen = 1, + .iVersion = 2, + .iAddressFamily = AF_INET, + .iMaxSockAddr = sizeof(struct sockaddr_in), + .iMinSockAddr = sizeof(struct sockaddr_in), + .iSocketType = SOCK_RAW, + .iProtocol = 0, + .iProtocolMaxOffset = 255, + .dwMessageSize = 0x8000, + .szProtocol = L"MSAFD Tcpip [RAW/IP]", + }, { .dwServiceFlags1 = XP1_IFS_HANDLES | XP1_EXPEDITED_DATA | XP1_GRACEFUL_CLOSE | XP1_GUARANTEED_ORDER | XP1_GUARANTEED_DELIVERY, @@ -4097,12 +4114,13 @@ int WINAPI WSARecvDisconnect( SOCKET s, WSABUF *data ) } -static BOOL protocol_matches_filter( const int *filter, int protocol ) +static BOOL protocol_matches_filter( const int *filter, unsigned int index ) { + if (supported_protocols[index].dwProviderFlags & PFL_HIDDEN) return FALSE; if (!filter) return TRUE; while (*filter) { - if (protocol == *filter++) return TRUE; + if (supported_protocols[index].iProtocol == *filter++) return TRUE; } return FALSE; } @@ -4120,7 +4138,7 @@ int WINAPI WSAEnumProtocolsA( int *filter, WSAPROTOCOL_INFOA *protocols, DWORD * for (i = 0; i < ARRAY_SIZE(supported_protocols); ++i) { - if (protocol_matches_filter( filter, supported_protocols[i].iProtocol )) + if (protocol_matches_filter( filter, i )) ++count; } @@ -4134,7 +4152,7 @@ int WINAPI WSAEnumProtocolsA( int *filter, WSAPROTOCOL_INFOA *protocols, DWORD * count = 0; for (i = 0; i < ARRAY_SIZE(supported_protocols); ++i) { - if (protocol_matches_filter( filter, supported_protocols[i].iProtocol )) + if (protocol_matches_filter( filter, i )) { memcpy( &protocols[count], &supported_protocols[i], offsetof( WSAPROTOCOL_INFOW, szProtocol ) ); WideCharToMultiByte( CP_ACP, 0, supported_protocols[i].szProtocol, -1, @@ -4190,7 +4208,7 @@ int WINAPI WSAEnumProtocolsW( int *filter, WSAPROTOCOL_INFOW *protocols, DWORD * for (i = 0; i < ARRAY_SIZE(supported_protocols); ++i) { - if (protocol_matches_filter( filter, supported_protocols[i].iProtocol )) + if (protocol_matches_filter( filter, i )) ++count; } @@ -4204,7 +4222,7 @@ int WINAPI WSAEnumProtocolsW( int *filter, WSAPROTOCOL_INFOW *protocols, DWORD * count = 0; for (i = 0; i < ARRAY_SIZE(supported_protocols); ++i) { - if (protocol_matches_filter( filter, supported_protocols[i].iProtocol )) + if (protocol_matches_filter( filter, i )) protocols[count++] = supported_protocols[i]; } return count; diff --git a/dlls/ws2_32/tests/protocol.c b/dlls/ws2_32/tests/protocol.c index 9a6c8610718..0b14543606a 100644 --- a/dlls/ws2_32/tests/protocol.c +++ b/dlls/ws2_32/tests/protocol.c @@ -107,6 +107,7 @@ static void test_WSAEnumProtocolsA(void) for (i = 0; i < ret; i++) { ok( strlen( buffer[i].szProtocol ), "No protocol name found\n" ); + ok( !(buffer[i].dwProviderFlags & PFL_HIDDEN), "Found a protocol with PFL_HIDDEN.\n" ); test_service_flags( buffer[i].iAddressFamily, buffer[i].iVersion, buffer[i].iSocketType, buffer[i].iProtocol, buffer[i].dwServiceFlags1); @@ -174,6 +175,7 @@ static void test_WSAEnumProtocolsW(void) for (i = 0; i < ret; i++) { ok( lstrlenW( buffer[i].szProtocol ), "No protocol name found\n" ); + ok( !(buffer[i].dwProviderFlags & PFL_HIDDEN), "Found a protocol with PFL_HIDDEN.\n" ); test_service_flags( buffer[i].iAddressFamily, buffer[i].iVersion, buffer[i].iSocketType, buffer[i].iProtocol, buffer[i].dwServiceFlags1); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 3ec0ab67040..f611fe6ed9d 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -2937,18 +2937,38 @@ static void test_WSASocket(void) } else { + WSAPROTOCOL_INFOW info; + size = sizeof(socktype); socktype = 0xdead; err = getsockopt(sock, SOL_SOCKET, SO_TYPE, (char *) &socktype, &size); ok(!err, "getsockopt failed with %d\n", WSAGetLastError()); ok(socktype == SOCK_RAW, "Wrong socket type, expected %d received %d\n", SOCK_RAW, socktype); + + size = sizeof(info); + err = getsockopt(sock, SOL_SOCKET, SO_PROTOCOL_INFOW, (char *) &info, &size); + ok(!err,"got error %d\n", WSAGetLastError()); + ok(!wcscmp(info.szProtocol, L"MSAFD Tcpip [RAW/IP]") + || broken(!wcscmp(info.szProtocol, L"MSAFD-Tcpip [RAW/IP]")) /* Some Win7 machines. */, + "got szProtocol %s.\n", debugstr_w(info.szProtocol)); + ok(info.iAddressFamily == AF_INET, "got iAddressFamily %d.\n", info.iAddressFamily); + ok(info.iSocketType == SOCK_RAW, "got iSocketType %d.\n", info.iSocketType); + ok(info.iMaxSockAddr == 0x10, "got iMaxSockAddr %d.\n", info.iMaxSockAddr); + ok(info.iMinSockAddr == 0x10, "got iMinSockAddr %d.\n", info.iMinSockAddr); + todo_wine ok(!info.iProtocol, "got iProtocol %d.\n", info.iProtocol); + ok(info.iProtocolMaxOffset == 255, "got iProtocol %d.\n", info.iProtocolMaxOffset); + ok(info.dwProviderFlags == (PFL_MATCHES_PROTOCOL_ZERO | PFL_HIDDEN), "got dwProviderFlags %#lx.\n", + info.dwProviderFlags); + ok(info.dwServiceFlags1 == (XP1_IFS_HANDLES | XP1_SUPPORT_BROADCAST | XP1_SUPPORT_MULTIPOINT + | XP1_MESSAGE_ORIENTED | XP1_CONNECTIONLESS), "got dwServiceFlags1 %#lx.\n", + info.dwServiceFlags1); + closesocket(sock); sock = WSASocketA(0, 0, IPPROTO_RAW, NULL, 0, 0); if (sock != INVALID_SOCKET) { - todo_wine { size = sizeof(socktype); socktype = 0xdead; err = getsockopt(sock, SOL_SOCKET, SO_TYPE, (char *) &socktype, &size); @@ -2956,7 +2976,6 @@ static void test_WSASocket(void) ok(socktype == SOCK_RAW, "Wrong socket type, expected %d received %d\n", SOCK_RAW, socktype); closesocket(sock); - } sock = WSASocketA(AF_INET, SOCK_RAW, IPPROTO_TCP, NULL, 0, 0); ok(sock != INVALID_SOCKET, "Failed to create socket: %d\n", -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/384
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/sock.c | 150 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 150 insertions(+) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index f611fe6ed9d..03677b72080 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -12816,6 +12816,155 @@ static void test_tcp_reset(void) CloseHandle(overlapped.hEvent); } +struct icmp_hdr +{ + BYTE type; + BYTE code; + UINT16 checksum; + union + { + struct + { + UINT16 id; + UINT16 sequence; + } echo; + } un; +}; + +struct ip_hdr +{ + BYTE v_hl; /* version << 4 | hdr_len */ + BYTE tos; + UINT16 tot_len; + UINT16 id; + UINT16 frag_off; + BYTE ttl; + BYTE protocol; + UINT16 checksum; + ULONG saddr; + ULONG daddr; +}; + +/* rfc 1071 checksum */ +static unsigned short chksum(BYTE *data, unsigned int count) +{ + unsigned int sum = 0, carry = 0; + unsigned short check, s; + + while (count > 1) + { + s = *(unsigned short *)data; + data += 2; + sum += carry; + sum += s; + carry = s > sum; + count -= 2; + } + sum += carry; /* This won't produce another carry */ + sum = (sum & 0xffff) + (sum >> 16); + + if (count) sum += *data; /* LE-only */ + + sum = (sum & 0xffff) + (sum >> 16); + /* fold in any carry */ + sum = (sum & 0xffff) + (sum >> 16); + + check = ~sum; + return check; +} + +static void test_icmp(const char *dest_addr) +{ + static const unsigned int ping_data = 0xdeadbeef; + struct icmp_hdr *icmp_h; + BYTE send_buf[sizeof(struct icmp_hdr) + sizeof(ping_data)]; + UINT16 recv_checksum, checksum; + unsigned int reply_data; + struct sockaddr_in sa; + struct ip_hdr *ip_h; + struct in_addr addr; + BYTE recv_buf[256]; + SOCKET s; + int ret; + + s = WSASocketA(AF_INET, SOCK_RAW, IPPROTO_ICMP, NULL, 0, 0); + if (s == INVALID_SOCKET) + { + ret = WSAGetLastError(); + ok(ret == WSAEACCES, "Expected 10013, received %d\n", ret); + skip("SOCK_RAW is not supported\n"); + return; + } + + icmp_h = (struct icmp_hdr *)send_buf; + icmp_h->type = ICMP4_ECHO_REQUEST; + icmp_h->code = 0; + icmp_h->checksum = 0; + icmp_h->un.echo.id = 0xbeaf; /* will be overwritten for linux ping socks */ + icmp_h->un.echo.sequence = 1; + *(unsigned int *)(icmp_h + 1) = ping_data; + icmp_h->checksum = chksum(send_buf, sizeof(send_buf)); + + memset(&sa, 0, sizeof(sa)); + sa.sin_family = AF_INET; + sa.sin_port = 0; + sa.sin_addr.s_addr = inet_addr(dest_addr); + + ret = sendto(s, (char *)send_buf, sizeof(send_buf), 0, (struct sockaddr*)&sa, sizeof(sa)); + ok(ret != SOCKET_ERROR, "got error %d.\n", WSAGetLastError()); + + ret = recv(s, (char *)recv_buf, sizeof(struct ip_hdr) + sizeof(send_buf) - 1, 0); + ok(ret == -1, "got %d\n", ret); + ok(WSAGetLastError() == WSAEMSGSIZE, "got %d\n", WSAGetLastError()); + + icmp_h->un.echo.sequence = 2; + icmp_h->checksum = 0; + icmp_h->checksum = chksum(send_buf, sizeof(send_buf)); + + ret = sendto(s, (char *)send_buf, sizeof(send_buf), 0, (struct sockaddr*)&sa, sizeof(sa)); + ok(ret != SOCKET_ERROR, "got error %d.\n", WSAGetLastError()); + + memset(recv_buf, 0xcc, sizeof(recv_buf)); + ret = recv(s, (char *)recv_buf, sizeof(recv_buf), 0); + ok(ret == sizeof(struct ip_hdr) + sizeof(send_buf), "got %d\n", ret); + + ip_h = (struct ip_hdr *)recv_buf; + icmp_h = (struct icmp_hdr *)(ip_h + 1); + reply_data = *(unsigned int *)(icmp_h + 1); + + ok(ip_h->v_hl == ((4 << 4) | (sizeof(*ip_h) >> 2)), "got v_hl %#x.\n", ip_h->v_hl); + ok(ntohs(ip_h->tot_len) == sizeof(struct ip_hdr) + sizeof(send_buf), + "got tot_len %#x.\n", ntohs(ip_h->tot_len)); + + recv_checksum = ip_h->checksum; + ip_h->checksum = 0; + checksum = chksum((BYTE *)ip_h, sizeof(*ip_h)); + /* Checksum is 0 for localhost ping on Windows but not for remote host ping. */ + ok(recv_checksum == checksum || !recv_checksum, "got checksum %#x, expected %#x.\n", recv_checksum, checksum); + + ok(!ip_h->frag_off, "got id %#x.\n", ip_h->frag_off); + addr.s_addr = ip_h->saddr; + ok(ip_h->saddr == sa.sin_addr.s_addr, "got saddr %s.\n", inet_ntoa(addr)); + addr.s_addr = ip_h->daddr; + ok(!!ip_h->daddr, "got daddr %s.\n", inet_ntoa(addr)); + + ok(ip_h->protocol == 1, "got protocol %#x.\n", ip_h->protocol); + + ok(icmp_h->type == ICMP4_ECHO_REPLY, "got type %#x.\n", icmp_h->type); + ok(!icmp_h->code, "got code %#x.\n", icmp_h->code); + todo_wine ok(icmp_h->un.echo.id == 0xbeaf, "got echo id %#x.\n", icmp_h->un.echo.id); + ok(icmp_h->un.echo.sequence == 2, "got echo sequence %#x.\n", icmp_h->un.echo.sequence); + + recv_checksum = icmp_h->checksum; + icmp_h->checksum = 0; + checksum = chksum((BYTE *)icmp_h, sizeof(send_buf)); + ok(recv_checksum == checksum, "got checksum %#x, expected %#x.\n", recv_checksum, checksum); + + ok(reply_data == ping_data, "got reply_data %#x.\n", reply_data); + + closesocket(s); +} + START_TEST( sock ) { int i; @@ -12892,6 +13041,7 @@ START_TEST( sock ) test_empty_recv(); test_timeout(); test_tcp_reset(); + test_icmp("127.0.0.1"); /* this is an io heavy test, do it at the end so the kernel doesn't start dropping packets */ test_send(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/384
On 7/5/22 14:53, Paul Gofman wrote:
From: Paul Gofman <pgofman(a)codeweavers.com>
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/tests/sock.c | 150 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 150 insertions(+)
I don't have much familiarity with ICMP, but...
+static void test_icmp(const char *dest_addr)
Any reason to pass this as a parameter? I'm guessing it's convenient to test ping over a real adapter, but presumably it can also just be edited inside the function...
+{ + static const unsigned int ping_data = 0xdeadbeef; + struct icmp_hdr *icmp_h; + BYTE send_buf[sizeof(struct icmp_hdr) + sizeof(ping_data)]; + UINT16 recv_checksum, checksum; + unsigned int reply_data; + struct sockaddr_in sa; + struct ip_hdr *ip_h; + struct in_addr addr; + BYTE recv_buf[256]; + SOCKET s; + int ret; + + s = WSASocketA(AF_INET, SOCK_RAW, IPPROTO_ICMP, NULL, 0, 0); + if (s == INVALID_SOCKET) + { + ret = WSAGetLastError(); + ok(ret == WSAEACCES, "Expected 10013, received %d\n", ret); + skip("SOCK_RAW is not supported\n"); + return; + }
To clarify, this should never trip on Windows machines; it's only here for Linux machines that don't support unprivileged ICMP, right? If so a comment to that effect would be helpful. I also don't know if we want to use a todo_wine in that case instead of skip().
+ + icmp_h = (struct icmp_hdr *)send_buf; + icmp_h->type = ICMP4_ECHO_REQUEST; + icmp_h->code = 0; + icmp_h->checksum = 0; + icmp_h->un.echo.id = 0xbeaf; /* will be overwritten for linux ping socks */ + icmp_h->un.echo.sequence = 1; + *(unsigned int *)(icmp_h + 1) = ping_data; + icmp_h->checksum = chksum(send_buf, sizeof(send_buf)); + + memset(&sa, 0, sizeof(sa)); + sa.sin_family = AF_INET; + sa.sin_port = 0; + sa.sin_addr.s_addr = inet_addr(dest_addr); + + ret = sendto(s, (char *)send_buf, sizeof(send_buf), 0, (struct sockaddr*)&sa, sizeof(sa)); + ok(ret != SOCKET_ERROR, "got error %d.\n", WSAGetLastError());
Any reason not to just say "ok(ret == sizeof(send_buf)"?
On 7/8/22 17:25, Zebediah Figura wrote:
Any reason to pass this as a parameter? I'm guessing it's convenient to test ping over a real adapter, but presumably it can also just be edited inside the function...
Sure.
+{ + static const unsigned int ping_data = 0xdeadbeef; + struct icmp_hdr *icmp_h; + BYTE send_buf[sizeof(struct icmp_hdr) + sizeof(ping_data)]; + UINT16 recv_checksum, checksum; + unsigned int reply_data; + struct sockaddr_in sa; + struct ip_hdr *ip_h; + struct in_addr addr; + BYTE recv_buf[256]; + SOCKET s; + int ret; + + s = WSASocketA(AF_INET, SOCK_RAW, IPPROTO_ICMP, NULL, 0, 0); + if (s == INVALID_SOCKET) + { + ret = WSAGetLastError(); + ok(ret == WSAEACCES, "Expected 10013, received %d\n", ret); + skip("SOCK_RAW is not supported\n"); + return; + }
To clarify, this should never trip on Windows machines; it's only here for Linux machines that don't support unprivileged ICMP, right? If so a comment to that effect would be helpful. I also don't know if we want to use a todo_wine in that case instead of skip().
I think I see this returning WSAEACCES on some testbot machines (most interestingly, some older admin ones). But not the "regular" machines. So I guess it should be skipped for Testbot as well.
+ + icmp_h = (struct icmp_hdr *)send_buf; + icmp_h->type = ICMP4_ECHO_REQUEST; + icmp_h->code = 0; + icmp_h->checksum = 0; + icmp_h->un.echo.id = 0xbeaf; /* will be overwritten for linux ping socks */ + icmp_h->un.echo.sequence = 1; + *(unsigned int *)(icmp_h + 1) = ping_data; + icmp_h->checksum = chksum(send_buf, sizeof(send_buf)); + + memset(&sa, 0, sizeof(sa)); + sa.sin_family = AF_INET; + sa.sin_port = 0; + sa.sin_addr.s_addr = inet_addr(dest_addr); + + ret = sendto(s, (char *)send_buf, sizeof(send_buf), 0, (struct sockaddr*)&sa, sizeof(sa)); + ok(ret != SOCKET_ERROR, "got error %d.\n", WSAGetLastError());
Any reason not to just say "ok(ret == sizeof(send_buf)"?
Sure.
On 2022-07-09 01:25, Zebediah Figura wrote:
On 7/5/22 14:53, Paul Gofman wrote: ...
+static void test_icmp(const char *dest_addr)
Any reason to pass this as a parameter? I'm guessing it's convenient to test ping over a real adapter, but presumably it can also just be edited inside the function...
Now it's doing:
+ test_icmp("127.0.0.1");
What if someone adds a real adapter identifying logic and invokes test for it too? Eg. to see the differences between it and the loopback. Will it require to duplicate all of the function's body? This would seem like a wasted efforts to me. It does at least without having actual differences at hand (proving to be too many inconsistencies between both runs and it being worth to rewrite the body for a real adapter). S.
On 7/9/22 02:47, Saulius Krasuckas wrote:
On 2022-07-09 01:25, Zebediah Figura wrote:
On 7/5/22 14:53, Paul Gofman wrote: ...
+static void test_icmp(const char *dest_addr)
Any reason to pass this as a parameter? I'm guessing it's convenient to test ping over a real adapter, but presumably it can also just be edited inside the function...
Now it's doing:
+ test_icmp("127.0.0.1");
What if someone adds a real adapter identifying logic and invokes test for it too? Eg. to see the differences between it and the loopback. Will it require to duplicate all of the function's body? This would seem like a wasted efforts to me. It does at least without having actual differences at hand (proving to be too many inconsistencies between both runs and it being worth to rewrite the body for a real adapter).
No, why would it? It'd be simple to turn that back into a parameter.
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/socket.c | 112 +++++++++++++++++++++++++++++++++++---- server/protocol.def | 8 ++- server/sock.c | 32 +++++++++++ 3 files changed, 140 insertions(+), 12 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 351028d4983..d200c0c999c 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -115,6 +115,7 @@ struct async_recv_ioctl DWORD *ret_flags; int unix_flags; unsigned int count; + enum sock_prot_fixup_type fixup_type; struct iovec iov[1]; }; @@ -566,6 +567,20 @@ static int wow64_translate_control( const WSABUF *control64, struct afd_wsabuf_3 return 1; } +struct ip_hdr +{ + BYTE v_hl; /* version << 4 | hdr_len */ + BYTE tos; + UINT16 tot_len; + UINT16 id; + UINT16 frag_off; + BYTE ttl; + BYTE protocol; + UINT16 checksum; + ULONG saddr; + ULONG daddr; +}; + static NTSTATUS try_recv( int fd, struct async_recv_ioctl *async, ULONG_PTR *size ) { #ifndef HAVE_STRUCT_MSGHDR_MSG_ACCRIGHTS @@ -576,8 +591,14 @@ static NTSTATUS try_recv( int fd, struct async_recv_ioctl *async, ULONG_PTR *siz NTSTATUS status; ssize_t ret; + if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM && async->count != 1) + { + FIXME( "async->count %d not supported for ICMP fixup.\n", async->count ); + async->fixup_type = SOCK_PROT_FIXUP_NONE; + } + memset( &hdr, 0, sizeof(hdr) ); - if (async->addr) + if (async->addr || async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) { hdr.msg_name = &unix_addr.addr; hdr.msg_namelen = sizeof(unix_addr); @@ -602,9 +623,68 @@ static NTSTATUS try_recv( int fd, struct async_recv_ioctl *async, ULONG_PTR *siz } status = (hdr.msg_flags & MSG_TRUNC) ? STATUS_BUFFER_OVERFLOW : STATUS_SUCCESS; + if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) + { + unsigned int tot_len = sizeof(struct ip_hdr) + ret; + size_t len = hdr.msg_iov[0].iov_len; + char *buf = hdr.msg_iov[0].iov_base; + struct cmsghdr *cmsg; + struct ip_hdr ip_h; + + if (ret + sizeof(ip_h) > len) + status = STATUS_BUFFER_OVERFLOW; + + if (len < sizeof(ip_h)) + { + ret = len; + } + else + { + ret = min( ret, len - sizeof(ip_h) ); + memmove( buf + sizeof(ip_h), buf, ret ); + ret += sizeof(ip_h); + } + memset( &ip_h, 0, sizeof(ip_h) ); + ip_h.v_hl = (4 << 4) | (sizeof(ip_h) >> 2); + ip_h.tot_len = htons( tot_len ); + ip_h.protocol = 1; + ip_h.saddr = unix_addr.in.sin_addr.s_addr; + + for (cmsg = CMSG_FIRSTHDR( &hdr ); cmsg; cmsg = CMSG_NXTHDR( &hdr, cmsg )) + { + if (cmsg->cmsg_level != IPPROTO_IP) continue; + switch (cmsg->cmsg_type) + { +#if defined(IP_TTL) + case IP_TTL: + ip_h.ttl = *(BYTE *)CMSG_DATA( cmsg ); + break; +#endif +#if defined(IP_TOS) + case IP_TOS: + ip_h.tos = *(BYTE *)CMSG_DATA( cmsg ); + break; +#endif +#if defined(IP_PKTINFO) + case IP_PKTINFO: + { + struct in_pktinfo *info; + + info = (struct in_pktinfo *)CMSG_DATA( cmsg ); + ip_h.daddr = info->ipi_addr.s_addr; + break; + } +#endif + } + } + memcpy( buf, &ip_h, min( sizeof(ip_h), len )); + } if (async->control) { + if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) + FIXME( "May return extra control headers.\n" ); + if (in_wow64_call()) { char control_buffer64[512]; @@ -727,6 +807,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi async->addr = addr; async->addr_len = addr_len; async->ret_flags = ret_flags; + async->fixup_type = SOCK_PROT_FIXUP_NONE; for (i = 0; i < count; ++i) { @@ -737,17 +818,26 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi } } - SERVER_START_REQ( recv_socket ) + do { - req->force_async = force_async; - req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); - req->oob = !!(unix_flags & MSG_OOB); - status = wine_server_call( req ); - wait_handle = wine_server_ptr_handle( reply->wait ); - options = reply->options; - nonblocking = reply->nonblocking; - } - SERVER_END_REQ; + SERVER_START_REQ( recv_socket ) + { + req->force_async = force_async; + req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); + req->oob = !!(unix_flags & MSG_OOB); + req->fixup_type = async->fixup_type; + status = wine_server_call( req ); + if (status == STATUS_MORE_PROCESSING_REQUIRED) + { + async->fixup_type = reply->fixup_type; + continue; + } + wait_handle = wine_server_ptr_handle( reply->wait ); + options = reply->options; + nonblocking = reply->nonblocking; + } + SERVER_END_REQ; + } while (status == STATUS_MORE_PROCESSING_REQUIRED); alerted = status == STATUS_ALERTED; if (alerted) diff --git a/server/protocol.def b/server/protocol.def index a8beccaf468..e1e5f3d9faa 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1446,17 +1446,23 @@ enum server_fd_type file_pos_t count; /* count of bytes to unlock */ @END - /* Perform a recv on a socket */ @REQ(recv_socket) int oob; /* are we receiving OOB data? */ async_data_t async; /* async I/O parameters */ int force_async; /* Force asynchronous mode? */ + int fixup_type; /* protocol fixup type returned previously */ @REPLY obj_handle_t wait; /* handle to wait on for blocking recv */ unsigned int options; /* device open options */ int nonblocking; /* is socket non-blocking? */ + int fixup_type; /* protocol fixup type (see below) */ @END +enum sock_prot_fixup_type +{ + SOCK_PROT_FIXUP_NONE, + SOCK_PROT_FIXUP_ICMP_OVER_DGRAM, +}; /* Perform a send on a socket */ diff --git a/server/sock.c b/server/sock.c index 2f1b33a333d..5c09c3bff3b 100644 --- a/server/sock.c +++ b/server/sock.c @@ -216,6 +216,7 @@ struct sock unsigned int sndbuf; /* advisory send buffer size */ unsigned int rcvtimeo; /* receive timeout in ms */ unsigned int sndtimeo; /* send timeout in ms */ + enum sock_prot_fixup_type fixup_type; /* protocol fixup performed */ unsigned int rd_shutdown : 1; /* is the read end shut down? */ unsigned int wr_shutdown : 1; /* is the write end shut down? */ unsigned int wr_shutdown_pending : 1; /* is a write shutdown pending? */ @@ -1551,6 +1552,7 @@ static struct sock *create_socket(void) sock->sndbuf = 0; sock->rcvtimeo = 0; sock->sndtimeo = 0; + sock->fixup_type = SOCK_PROT_FIXUP_NONE; init_async_queue( &sock->read_q ); init_async_queue( &sock->write_q ); init_async_queue( &sock->ifchange_q ); @@ -1667,6 +1669,24 @@ static int init_socket( struct sock *sock, int family, int type, int protocol, u } sockfd = socket( unix_family, unix_type, unix_protocol ); + +#ifdef linux + if (sockfd == -1 && errno == EPERM && unix_family == AF_INET + && unix_type == SOCK_RAW && unix_protocol == IPPROTO_ICMP) + { + sockfd = socket( unix_family, SOCK_DGRAM, unix_protocol ); + if (sockfd != -1) + { + const int val = 1; + + sock->fixup_type = SOCK_PROT_FIXUP_ICMP_OVER_DGRAM; + setsockopt( sockfd, IPPROTO_IP, IP_RECVTTL, (const char *)&val, sizeof(val) ); + setsockopt( sockfd, IPPROTO_IP, IP_RECVTOS, (const char *)&val, sizeof(val) ); + setsockopt( sockfd, IPPROTO_IP, IP_PKTINFO, (const char *)&val, sizeof(val) ); + } + } +#endif + if (sockfd == -1) { if (errno == EINVAL) set_win32_error( WSAESOCKTNOSUPPORT ); @@ -3459,6 +3479,18 @@ DECL_HANDLER(recv_socket) struct fd *fd; if (!sock) return; + + if (sock->fixup_type != req->fixup_type) + { + /* Protocol fixup information should be reflected in client's async data before the + * async is delivered to process. So let the client set up the info and restart the + * call. */ + reply->fixup_type = sock->fixup_type; + set_error( STATUS_MORE_PROCESSING_REQUIRED ); + release_object( sock ); + return; + } + fd = sock->fd; if (!req->force_async && !sock->nonblocking && is_fd_overlapped( fd )) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/384
On 7/5/22 14:53, Paul Gofman wrote: @@ -602,9 +623,68 @@ static NTSTATUS try_recv( int fd, struct async_recv_ioctl *async, ULONG_PTR *siz } status = (hdr.msg_flags & MSG_TRUNC) ? STATUS_BUFFER_OVERFLOW : STATUS_SUCCESS; + if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) + { + unsigned int tot_len = sizeof(struct ip_hdr) + ret; + size_t len = hdr.msg_iov[0].iov_len; + char *buf = hdr.msg_iov[0].iov_base; + struct cmsghdr *cmsg; + struct ip_hdr ip_h; + + if (ret + sizeof(ip_h) > len) + status = STATUS_BUFFER_OVERFLOW; + + if (len < sizeof(ip_h)) + { + ret = len; + } + else + { + ret = min( ret, len - sizeof(ip_h) ); + memmove( buf + sizeof(ip_h), buf, ret ); + ret += sizeof(ip_h); + } + memset( &ip_h, 0, sizeof(ip_h) ); + ip_h.v_hl = (4 << 4) | (sizeof(ip_h) >> 2); + ip_h.tot_len = htons( tot_len ); + ip_h.protocol = 1; + ip_h.saddr = unix_addr.in.sin_addr.s_addr; + + for (cmsg = CMSG_FIRSTHDR( &hdr ); cmsg; cmsg = CMSG_NXTHDR( &hdr, cmsg )) + { + if (cmsg->cmsg_level != IPPROTO_IP) continue; + switch (cmsg->cmsg_type) + { +#if defined(IP_TTL) + case IP_TTL: + ip_h.ttl = *(BYTE *)CMSG_DATA( cmsg ); + break; +#endif +#if defined(IP_TOS) + case IP_TOS: + ip_h.tos = *(BYTE *)CMSG_DATA( cmsg ); + break; +#endif +#if defined(IP_PKTINFO) + case IP_PKTINFO: + { + struct in_pktinfo *info; + + info = (struct in_pktinfo *)CMSG_DATA( cmsg ); + ip_h.daddr = info->ipi_addr.s_addr; + break; + } +#endif + } + } + memcpy( buf, &ip_h, min( sizeof(ip_h), len )); + } Would you mind making this a helper function? try_recv() is already large and this seems to be pretty self-contained logic.
if (async->control) { + if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) + FIXME( "May return extra control headers.\n" ); +
What is this for?
@@ -737,17 +818,26 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi } }
- SERVER_START_REQ( recv_socket ) + do { - req->force_async = force_async; - req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); - req->oob = !!(unix_flags & MSG_OOB); - status = wine_server_call( req ); - wait_handle = wine_server_ptr_handle( reply->wait ); - options = reply->options; - nonblocking = reply->nonblocking; - } - SERVER_END_REQ; + SERVER_START_REQ( recv_socket ) + { + req->force_async = force_async; + req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); + req->oob = !!(unix_flags & MSG_OOB); + req->fixup_type = async->fixup_type; + status = wine_server_call( req ); + if (status == STATUS_MORE_PROCESSING_REQUIRED) + { + async->fixup_type = reply->fixup_type; + continue; + } + wait_handle = wine_server_ptr_handle( reply->wait ); + options = reply->options; + nonblocking = reply->nonblocking; + } + SERVER_END_REQ; + } while (status == STATUS_MORE_PROCESSING_REQUIRED);
alerted = status == STATUS_ALERTED; if (alerted)
I'm missing something; why do we need to call this in a loop? Why does the client needs to communicate the fixup type to the server in the first place?
diff --git a/server/protocol.def b/server/protocol.def index a8beccaf468..e1e5f3d9faa 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1446,17 +1446,23 @@ enum server_fd_type file_pos_t count; /* count of bytes to unlock */ @END
- /* Perform a recv on a socket */ @REQ(recv_socket) int oob; /* are we receiving OOB data? */ async_data_t async; /* async I/O parameters */ int force_async; /* Force asynchronous mode? */ + int fixup_type; /* protocol fixup type returned previously */ @REPLY obj_handle_t wait; /* handle to wait on for blocking recv */ unsigned int options; /* device open options */ int nonblocking; /* is socket non-blocking? */ + int fixup_type; /* protocol fixup type (see below) */ @END +enum sock_prot_fixup_type +{ + SOCK_PROT_FIXUP_NONE, + SOCK_PROT_FIXUP_ICMP_OVER_DGRAM, +};
Making this an enum strikes me as odd in general. A bitmask might make more sense, but frankly I'd just leave it as a dedicated field until we have something else that needs fixups. Do you have any plans for introducing other types of fixups?
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/socket.c | 95 ++++++++++++++++++++++++++++++++++++++++ dlls/ws2_32/tests/sock.c | 2 +- server/protocol.def | 23 +++++++++- server/sock.c | 70 +++++++++++++++++++++++++++++ 4 files changed, 187 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index d200c0c999c..d19efba14fa 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -581,6 +581,49 @@ struct ip_hdr ULONG daddr; }; +struct icmp_hdr +{ + BYTE type; + BYTE code; + UINT16 checksum; + union + { + struct + { + UINT16 id; + UINT16 sequence; + } echo; + } un; +}; + +/* rfc 1071 checksum */ +static unsigned short chksum(BYTE *data, unsigned int count) +{ + unsigned int sum = 0, carry = 0; + unsigned short check, s; + + while (count > 1) + { + s = *(unsigned short *)data; + data += 2; + sum += carry; + sum += s; + carry = s > sum; + count -= 2; + } + sum += carry; /* This won't produce another carry */ + sum = (sum & 0xffff) + (sum >> 16); + + if (count) sum += *data; /* LE-only */ + + sum = (sum & 0xffff) + (sum >> 16); + /* fold in any carry */ + sum = (sum & 0xffff) + (sum >> 16); + + check = ~sum; + return check; +} + static NTSTATUS try_recv( int fd, struct async_recv_ioctl *async, ULONG_PTR *size ) { #ifndef HAVE_STRUCT_MSGHDR_MSG_ACCRIGHTS @@ -628,6 +671,8 @@ static NTSTATUS try_recv( int fd, struct async_recv_ioctl *async, ULONG_PTR *siz unsigned int tot_len = sizeof(struct ip_hdr) + ret; size_t len = hdr.msg_iov[0].iov_len; char *buf = hdr.msg_iov[0].iov_base; + struct icmp_hdr *icmp_h = NULL; + NTSTATUS fixup_status; struct cmsghdr *cmsg; struct ip_hdr ip_h; @@ -642,6 +687,8 @@ static NTSTATUS try_recv( int fd, struct async_recv_ioctl *async, ULONG_PTR *siz { ret = min( ret, len - sizeof(ip_h) ); memmove( buf + sizeof(ip_h), buf, ret ); + if (ret >= sizeof(struct icmp_hdr)) + icmp_h = (struct icmp_hdr *)(buf + sizeof(ip_h)); ret += sizeof(ip_h); } memset( &ip_h, 0, sizeof(ip_h) ); @@ -677,6 +724,24 @@ static NTSTATUS try_recv( int fd, struct async_recv_ioctl *async, ULONG_PTR *siz #endif } } + if (icmp_h) + { + SERVER_START_REQ( socket_get_fixup_data ) + { + req->handle = wine_server_obj_handle( async->io.handle ); + req->icmp_seq = icmp_h->un.echo.sequence; + if (!(fixup_status = wine_server_call( req ))) + icmp_h->un.echo.id = reply->icmp_id; + else + WARN( "socket_get_fixup_data returned %#x.\n", fixup_status ); + } + SERVER_END_REQ; + if (!fixup_status) + { + icmp_h->checksum = 0; + icmp_h->checksum = chksum( (BYTE *)icmp_h, ret - sizeof(ip_h) ); + } + } memcpy( buf, &ip_h, min( sizeof(ip_h), len )); } @@ -957,6 +1022,7 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi IO_STATUS_BLOCK *io, int fd, const void *buffers_ptr, unsigned int count, const struct WS_sockaddr *addr, unsigned int addr_len, int unix_flags, int force_async ) { + enum sock_prot_fixup_type fixup_type; struct async_send_ioctl *async; ULONG_PTR information; HANDLE wait_handle; @@ -1006,9 +1072,38 @@ static NTSTATUS sock_send( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi wait_handle = wine_server_ptr_handle( reply->wait ); options = reply->options; nonblocking = reply->nonblocking; + fixup_type = reply->fixup_type; } SERVER_END_REQ; + if (fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM && async->count + && (!status || status == STATUS_ALERTED || status == STATUS_PENDING)) + { + unsigned short id, seq; + struct icmp_hdr *h; + NTSTATUS ret; + + if (async->iov[0].iov_len < sizeof(*h)) + { + FIXME( "ICMP over DGRAM fixup is not supported for count %u, len %zu.\n", count, async->iov[0].iov_len ); + } + else + { + h = async->iov[0].iov_base; + id = h->un.echo.id; + seq = h->un.echo.sequence; + SERVER_START_REQ( socket_fixup_send_data ) + { + req->handle = wine_server_obj_handle( handle ); + req->icmp_id = id; + req->icmp_seq = seq; + ret = wine_server_call( req ); + } + SERVER_END_REQ; + if (ret) WARN( "socket_fixup_send_data returned %#x.\n", ret ); + } + } + alerted = status == STATUS_ALERTED; if (alerted) { diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 03677b72080..7e64c52c16b 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -12952,7 +12952,7 @@ static void test_icmp(const char *dest_addr) ok(icmp_h->type == ICMP4_ECHO_REPLY, "got type %#x.\n", icmp_h->type); ok(!icmp_h->code, "got code %#x.\n", icmp_h->code); - todo_wine ok(icmp_h->un.echo.id == 0xbeaf, "got echo id %#x.\n", icmp_h->un.echo.id); + ok(icmp_h->un.echo.id == 0xbeaf, "got echo id %#x.\n", icmp_h->un.echo.id); ok(icmp_h->un.echo.sequence == 2, "got echo sequence %#x.\n", icmp_h->un.echo.sequence); recv_checksum = icmp_h->checksum; diff --git a/server/protocol.def b/server/protocol.def index e1e5f3d9faa..ba367215bcd 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1467,12 +1467,31 @@ enum sock_prot_fixup_type /* Perform a send on a socket */ @REQ(send_socket) - async_data_t async; /* async I/O parameters */ - int force_async; /* Force asynchronous mode? */ + async_data_t async; /* async I/O parameters */ + int force_async; /* Force asynchronous mode? */ @REPLY obj_handle_t wait; /* handle to wait on for blocking send */ unsigned int options; /* device open options */ int nonblocking; /* is socket non-blocking? */ + int fixup_type; /* socket protocol fixup */ +(a)END + + +/* Store info needed for protocol fixup */ +(a)REQ(socket_fixup_send_data) + obj_handle_t handle; /* socket handle */ + unsigned short icmp_id; /* ICMP packet id */ + unsigned short icmp_seq; /* ICMP packed sequence */ +(a)REPLY +(a)END + + +/* Retrieve protocol fixup info */ +(a)REQ(socket_get_fixup_data) + obj_handle_t handle; /* socket handle */ + unsigned short icmp_seq; /* ICMP packed sequence */ +(a)REPLY + unsigned short icmp_id; /* ICMP packet id */ @END diff --git a/server/sock.c b/server/sock.c index 5c09c3bff3b..7fb4bd6c2f6 100644 --- a/server/sock.c +++ b/server/sock.c @@ -168,6 +168,8 @@ enum connection_state SOCK_CONNECTIONLESS, }; +#define MAX_ICMP_HISTORY_LENGTH 8 + struct sock { struct object obj; /* object header */ @@ -217,6 +219,13 @@ struct sock unsigned int rcvtimeo; /* receive timeout in ms */ unsigned int sndtimeo; /* send timeout in ms */ enum sock_prot_fixup_type fixup_type; /* protocol fixup performed */ + struct + { + unsigned short icmp_id; + unsigned short icmp_seq; + } + icmp_fixup_data[MAX_ICMP_HISTORY_LENGTH]; /* Sent ICMP packets history used to fixup reply id. */ + unsigned int icmp_fixup_data_len; /* Sent ICMP packets history length. */ unsigned int rd_shutdown : 1; /* is the read end shut down? */ unsigned int wr_shutdown : 1; /* is the write end shut down? */ unsigned int wr_shutdown_pending : 1; /* is a write shutdown pending? */ @@ -1553,6 +1562,7 @@ static struct sock *create_socket(void) sock->rcvtimeo = 0; sock->sndtimeo = 0; sock->fixup_type = SOCK_PROT_FIXUP_NONE; + sock->icmp_fixup_data_len = 0; init_async_queue( &sock->read_q ); init_async_queue( &sock->write_q ); init_async_queue( &sock->ifchange_q ); @@ -3574,6 +3584,8 @@ DECL_HANDLER(send_socket) if (!sock) return; fd = sock->fd; + reply->fixup_type = sock->fixup_type; + if (sock->type == WS_SOCK_DGRAM && !sock->bound) { union unix_sockaddr unix_addr; @@ -3653,3 +3665,61 @@ DECL_HANDLER(send_socket) } release_object( sock ); } + +DECL_HANDLER(socket_fixup_send_data) +{ + struct sock *sock = (struct sock *)get_handle_obj( current->process, req->handle, 0, &sock_ops ); + + if (!sock) return; + + if (sock->fixup_type != SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) + { + set_error( STATUS_INVALID_PARAMETER ); + release_object( sock ); + return; + } + + if (sock->icmp_fixup_data_len == MAX_ICMP_HISTORY_LENGTH) + { + set_error( STATUS_BUFFER_OVERFLOW ); + release_object( sock ); + return; + } + + sock->icmp_fixup_data[sock->icmp_fixup_data_len].icmp_id = req->icmp_id; + sock->icmp_fixup_data[sock->icmp_fixup_data_len].icmp_seq = req->icmp_seq; + ++sock->icmp_fixup_data_len; + + release_object( sock ); +} + +DECL_HANDLER(socket_get_fixup_data) +{ + struct sock *sock = (struct sock *)get_handle_obj( current->process, req->handle, 0, &sock_ops ); + unsigned int i; + + if (!sock) return; + + if (sock->fixup_type != SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) + { + set_error( STATUS_INVALID_PARAMETER ); + release_object( sock ); + return; + } + + for (i = 0; i < sock->icmp_fixup_data_len; ++i) + { + if (sock->icmp_fixup_data[i].icmp_seq == req->icmp_seq) + { + reply->icmp_id = sock->icmp_fixup_data[i].icmp_id; + --sock->icmp_fixup_data_len; + memmove( &sock->icmp_fixup_data[i], &sock->icmp_fixup_data[i + 1], + (sock->icmp_fixup_data_len - i) * sizeof(*sock->icmp_fixup_data) ); + release_object( sock ); + return; + } + } + + set_error( STATUS_NOT_FOUND ); + release_object( sock ); +} -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/384
On 7/5/22 14:53, Paul Gofman wrote:
From: Paul Gofman <pgofman(a)codeweavers.com>
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/socket.c | 95 ++++++++++++++++++++++++++++++++++++++++ dlls/ws2_32/tests/sock.c | 2 +- server/protocol.def | 23 +++++++++- server/sock.c | 70 +++++++++++++++++++++++++++++ 4 files changed, 187 insertions(+), 3 deletions(-)
This is awfully complicated. Would it be possible to just get the kernel to give us the right ID back, perhaps by adding a new socket option? For that matter, I guess the same goes for the first part of the series—if we could get a way to get a SOCK_RAW + IPPROTO_ICMP socket, presumably one where the kernel filters out anything unsafe, that'd be a lot nicer than fixing things up in Wine...
On 7/8/22 17:24, Zebediah Figura wrote:
This is awfully complicated. Would it be possible to just get the kernel to give us the right ID back, perhaps by adding a new socket option?
I honestly have no idea how easy is to change anything in Linux kernel in this regard but my guess is that not easy at all. I think kernel does not have that ID when receiving the reply packet. It fixes up the ID in the sent ICMP packet. And I guess it does it for security reasons, probably to make sure the unprivileged user can't interfere with other ping users (including root) by spoofing the ICMP ID which might interefere with them. So given how that was done already I doubt the change allowing to do what we want in a straightforward way can be accepted.
For that matter, I guess the same goes for the first part of the series—if we could get a way to get a SOCK_RAW + IPPROTO_ICMP socket, presumably one where the kernel filters out anything unsafe, that'd be a lot nicer than fixing things up in Wine...
Same here. For some reason that was made on top of DGRAM instead of allowing ICMP on RAW. I guess that probably to hide raw IP header instead of screening / fixing up it for security reasons (and maybe something else to it which I not immediately know of).
On 7/8/22 18:05, Paul Gofman wrote:
On 7/8/22 17:24, Zebediah Figura wrote:
This is awfully complicated. Would it be possible to just get the kernel to give us the right ID back, perhaps by adding a new socket option?
I honestly have no idea how easy is to change anything in Linux kernel in this regard but my guess is that not easy at all. I think kernel does not have that ID when receiving the reply packet. It fixes up the ID in the sent ICMP packet. And I guess it does it for security reasons, probably to make sure the unprivileged user can't interfere with other ping users (including root) by spoofing the ICMP ID which might interefere with them. So given how that was done already I doubt the change allowing to do what we want in a straightforward way can be accepted.
Hmm, okay. I feel like there's potentially an argument for "if you're going to fix it up, can you please fix it up on both ends so it's transparent to the user", but maybe it's not a compelling one. I guess the thing is, if there's a way to modify the kernel, even if it's just for Wine, and it's less ugly than modifying Wine itself, it may be worth looking into. It's not the first time that we've had to introduce options into the kernel (although granted, the only other case I know of offhand was something that I believe couldn't have been done in user space, viz. binding UDP sockets to a specific interface).
For that matter, I guess the same goes for the first part of the series—if we could get a way to get a SOCK_RAW + IPPROTO_ICMP socket, presumably one where the kernel filters out anything unsafe, that'd be a lot nicer than fixing things up in Wine...
Same here. For some reason that was made on top of DGRAM instead of allowing ICMP on RAW. I guess that probably to hide raw IP header instead of screening / fixing up it for security reasons (and maybe something else to it which I not immediately know of).
Yeah, it'd be nice to know if there is an underlying reason that the kernel doesn't want to just filter out unsafe headers, though, since as above that'd be a lot easier for us.
On 7/9/22 07:24, Zebediah Figura wrote:
On 7/5/22 14:53, Paul Gofman wrote:
From: Paul Gofman <pgofman(a)codeweavers.com>
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/socket.c | 95 ++++++++++++++++++++++++++++++++++++++++ dlls/ws2_32/tests/sock.c | 2 +- server/protocol.def | 23 +++++++++- server/sock.c | 70 +++++++++++++++++++++++++++++ 4 files changed, 187 insertions(+), 3 deletions(-)
This is awfully complicated. Would it be possible to just get the kernel to give us the right ID back, perhaps by adding a new socket option?
For that matter, I guess the same goes for the first part of the series—if we could get a way to get a SOCK_RAW + IPPROTO_ICMP socket, presumably one where the kernel filters out anything unsafe, that'd be a lot nicer than fixing things up in Wine...
Alternatively, we can use this as an excuse to implement (at least part of) Jacek's proposal, provided that the bugfix isn't urgent:
Did you consider moving whole sock_send() and try_send() to server? We could then have a proper and reliable queue in server socket object and client could just do a regular generic server ioctl. (I didn't really follow closely the conversation, so sorry if I missed something).
Since ICMP packets tend not to be too large, this would help simplify the fixup process. -- Sincerely, Jinoh Kang
On 7/5/22 14:53, Paul Gofman wrote:
+ if (fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM && async->count + && (!status || status == STATUS_ALERTED || status == STATUS_PENDING)) + { + unsigned short id, seq; + struct icmp_hdr *h; + NTSTATUS ret; + + if (async->iov[0].iov_len < sizeof(*h)) + { + FIXME( "ICMP over DGRAM fixup is not supported for count %u, len %zu.\n", count, async->iov[0].iov_len ); + } + else + { + h = async->iov[0].iov_base; + id = h->un.echo.id; + seq = h->un.echo.sequence; + SERVER_START_REQ( socket_fixup_send_data ) + { + req->handle = wine_server_obj_handle( handle ); + req->icmp_id = id; + req->icmp_seq = seq; + ret = wine_server_call( req ); + } + SERVER_END_REQ; + if (ret) WARN( "socket_fixup_send_data returned %#x.\n", ret ); + } + } +
Would you mind putting this in a helper function?
participants (6)
-
Jinoh Kang -
Paul Gofman -
Paul Gofman -
Paul Gofman (＠gofman)
-
Saulius Krasuckas -
Zebediah Figura