Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys/linux, executor: Add KSMBD subsystem support #5524

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions executor/common_linux.h
Original file line number Diff line number Diff line change
Expand Up @@ -5833,3 +5833,64 @@
}

#endif

#if SYZ_EXECUTOR || __NR_syz_ksmbd_send_req
#include <netinet/in.h>
#include <stdio.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/types.h>

#define KSMBD_BUF_SIZE 16000

static long syz_ksmbd_send_req(volatile long a0, volatile long a1, volatile long a2, volatile long a3)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan to read out the ksmdb responses and somehow act upon them?

If not, we probably don't really need to introduce a pseudo system call at all. If you pass the kcov remote reference the standard way, all that remains are:

  • A socket call.
  • A connect call with the specific arguments.
  • A write call.

These can be expressed in syzkaller descriptions and e.g. tied together by defining some custom resource sock_ksmbd[sock] resource.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for not adding a pseudo syscall. We just need to describe the tcp address, and add a special resource for the socket to tie all syscall together (socket/connect/sendmsg).

It has an additional advantage of giving more freedom to the fuzzer and exploring more scenarios. E.g. sending messages in parallel, calling shutdown, etc.

{
int sockfd;
int packet_reqlen;
int errno;

Check failure on line 5850 in executor/common_linux.h

View workflow job for this annotation

GitHub Actions / build

Don't use C89 var declarations. Declare vars where they are needed and combine with initialization

Check failure on line 5850 in executor/common_linux.h

View workflow job for this annotation

GitHub Actions / race

Don't use C89 var declarations. Declare vars where they are needed and combine with initialization
struct sockaddr_in serv_addr;
char packet_req[KSMBD_BUF_SIZE]; // max frame size

debug("[*]{syz_ksmbd_send_req} entered ksmbd send...\n");

if (a0 == 0 || a1 == 0) {
debug("[!]{syz_ksmbd_send_req} param empty\n");
return -7;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the fuzzer, it's not really important to distinguish between these specific failure paths. I would just return -1 for all of them.

}

sockfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
if (sockfd < 0) {
debug("[!]{syz_ksmbd_send_req} failed to create socket\n");
return -1;
}

memset(&serv_addr, '\0', sizeof(serv_addr));
serv_addr.sin_family = AF_INET;
serv_addr.sin_addr.s_addr = inet_addr("127.0.0.1");
serv_addr.sin_port = htons(445);

errno = connect(sockfd, (struct sockaddr*)&serv_addr, sizeof(serv_addr));
if (errno < 0) {
debug("[!]{syz_ksmbd_send_req} failed to connect (err: %d)\n", errno);
return errno ^ 0xff80000;
}

// prepend kcov handle to packet
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should not be necessary to pass a remote kcov reference explicitly within the packet. Syzkaller calls kcov_remote_start/kcov_remote_stop automatically, so the write handler on the kernel side should already be aware of the right kcov remote reference.

I understand that it would also require rewriting the patch to the kernel as well, so don't treat this comment as a blocker for merging this PR.

packet_reqlen = a1 + 8 > KSMBD_BUF_SIZE ? KSMBD_BUF_SIZE - 8 : a1;
*(unsigned long*)packet_req = procid + 1;
memcpy(packet_req + 8, (char*)a0, packet_reqlen);

if (write(sockfd, (char*)packet_req, packet_reqlen + 8) < 0)
return -4;

if (read(sockfd, (char*)a2, a3) < 0)
return -5;

if (close(sockfd) < 0)
return -6;

debug("[+]{syz_ksmbd_send_req} successfully returned\n");

return 0;
}
#endif
1 change: 1 addition & 0 deletions pkg/vminfo/linux_syscalls.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ var linuxSyscallChecks = map[string]func(*checkContext, *prog.Syscall) string{
"syz_socket_connect_nvme_tcp": linuxSyzSocketConnectNvmeTCPSupported,
"syz_pidfd_open": alwaysSupported,
"syz_create_resource": alwaysSupported,
"syz_ksmbd_send_req": alwaysSupported,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that for most users it will actually be not supported, so it would be nice to be more concrete here.

What is the simplest way of determining whether the server exists?

}

func linuxSyzOpenDevSupported(ctx *checkContext, call *prog.Syscall) string {
Expand Down
168 changes: 168 additions & 0 deletions sys/linux/ksmbd.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
include <linux/ip.h>
include <linux/ipv6.h>
include <linux/route.h>
include <uapi/linux/if_arp.h>
include <uapi/linux/netfilter_ipv6/ip6_tables.h>
include <uapi/linux/wireless.h>
include <uapi/linux/in.h>

sync_id {
ProcessId int32
TreeId int32
} [packed]

_union_smb_req_id [
sync_id sync_id
async_id int64
]

negotiate_message {
Signature array[int8, 8]
# NtLmNegotiate = 1
MessageType int32
NegotiateFlags int32
# MBZ unless SMB3.02 or later
DomainName_Length len[DomainString, int16]
DomainName_MaximumLength len[DomainString, int16]
DomainName_Offset offsetof[DomainString, int32]
# RFC 1001 and ASCII
WorkstationName_Length len[Workstationname_Buffer, int16]
WorkstationName_MaximumLength len[Workstationname_Buffer, int16]
WorkstationName_Offset offsetof[Workstationname_Buffer, int32]

# struct security_buffer for version info not present since we
# do not set the version is present flag
DomainString string

# followed by WorkstationString
Workstationname_Buffer string
} [packed]

_smb2_hdr_pre {
ProtocolId const[0xfe534d42, int32be]
StructureSize const[0x40, int16]
CreditCharge int16
Status int32
} [packed]

_smb2_hdr_post {
CreditRequest int16
Flags int32
NextCommand int32
MessageId int64
Id _union_smb_req_id
SessionId int64
Signature array[int8, 16]
} [packed]

#_smb3_hdr_pre {
# ProtocolId const[0xfe534d42, int32be]
# StructureSize const[0x40, int16]
# CreditCharge int16
# ChannelSequence int16
# Reserved int16
#} [packed]

#_smb3_hdr_post {
# CreditRequest int16
# Flags int32
# NextCommand int32
# MessageId int64
# Id _union_smb_req_id
# SessionId int64
# Signature array[int8, 16]
#} [packed]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these structs are not necessary, let's just drop them from the final descriptions.


smb2_negotiate_req {
_hdr_pre _smb2_hdr_pre
Command const[0x0, int16]
_hdr_post _smb2_hdr_post
StructureSize const[36, int16]
DialectCount len[Dialects, int16]
SecurityMode int16
Reserved int16
Capabilities int32
ClientGUID array[int8, 16]

# In SMB3.02 and earlier next three were MBZ le64 ClientStartTime
NegotiateContextOffset int32
NegotiateContextCount int16
Reserved2 int16
Dialects buffer[in]
} [packed]

smb2_sess_setup_req {
_hdr_pre _smb2_hdr_pre
Command const[0x1, int16]
_hdr_post _smb2_hdr_post
StructureSize const[25, int16]
Flags int8
SecurityMode int8
Capabilities int32
Channel int32
SecurityBufferOffset offsetof[Buffer, int16]
SecurityBufferLength len[Buffer, int16]
PreviousSessionId int64

# variable length GSS security buffer
Buffer negotiate_message
} [packed]

smb2_echo_req {
_hdr_pre _smb2_hdr_pre
Command const[0xd, int16]
_hdr_post _smb2_hdr_post
StructureSize2 const[4, int16]
Reserved int16
} [packed]

smbd_buffer_descriptor_v1 {
offset int64
token int32
length int32
# followed by Smbd_Buffer
smb2_read_req_buffer string
} [packed]

smb2_read_req {
_hdr_pre _smb2_hdr_pre
Command const[0x8, int16]
_hdr_post _smb2_hdr_post
# Must be 49
StructureSize const[49, int16]
# offset from start of SMB2 header to place read
Padding int8
# MBZ unless SMB3.02 or later
Flags int8
Length int32
Offset int64
PersistentFileId int64
VolatileFileId int64
MinimumCount int32
# MBZ except for SMB3 or later
Channel int32
RemainingBytes int32
ReadChannelInfoOffset offsetof[Buffer, int16]
ReadChannelInfoLength len[Buffer, int16]
Buffer smbd_buffer_descriptor_v1
} [packed]

_union_smb2_req [
smb2_negotiate_req smb2_negotiate_req
smb2_sess_setup_req smb2_sess_setup_req
smb2_echo_req smb2_echo_req
smb2_read_req smb2_read_req
] [varlen]

smb2_req {
req_base _union_smb2_req
req_andx1 optional[_union_smb2_req]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: if that depends on the specific values set in the header, please note that there's a way to express that in syzlang as well:

https://github.com/google/syzkaller/blob/master/docs/syscall_descriptions_syntax.md#conditional-fields

req_andx2 optional[_union_smb2_req]
} [packed]

smb2_req_pdu {
_pdu_size len[req, int32be]
req smb2_req
} [packed]

syz_ksmbd_send_req(req_packet ptr[in, smb2_req_pdu], req_len len[req_packet], res_packet buffer[out], res_len len[res_packet])
Loading