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.

Sign up for GitHub

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

Jump to bottom

Fix SCTP bind/connect mishandling #40

Open
sm1ling-knight opened this issue Sep 4, 2024 · 2 comments
Open

Fix SCTP bind/connect mishandling #40

sm1ling-knight opened this issue Sep 4, 2024 · 2 comments
Assignees
@sm1ling-knight
Labels
bug Something isn't working

Comments

@sm1ling-knight
Copy link

sm1ling-knight commented Sep 4, 2024 •
edited
Loading

SCTP is connection-oriented protocol that can be used to establish one-to-many and one-to-one communication between endpoints.

One-to-one style can be used by specifying AF_INET family, SOCK_STREAM type and IPPROTO_SCTP protocol value in the socket(2):

int sctp_client_fd;

sctp_client_fd = socket(AF_INET, SOCK_STREAM, IPPROTO_SCTP);

Current implementation of LANDLOCK_ACCESS_NET_BIND_TCP, LANDLOCK_ACCESS_NET_CONNECT_TCP allows to restrict bind/connect actions for both classic TCP sockets and SCTP sockets.

SCTP allows to bind and connect sockets not only with bind(2), connect(2), but also with setsockopt(3p). Options SCTP_SOCKOPT_CONNECT*, SCTP_SOCKOPT_BIND*, ... (Cf. SCTP) are provided for this purpose.

For example:

setsockopt(sctp_client_fd, IPPROTO_SCTP, SCTP_SOCKOPT_CONNECTX, &addr, sizeof(addr));

It is not possible to restrict such calls using LANDLOCK_ACCESS_NET_BIND_TCP, LANDLOCK_ACCESS_NET_CONNECT_TCP which leads to inconsistency of Landlock behavior.

There are a few ways to fix this issue:

  1. Change behavior of TCP access rights so that they check only classic TCP sockets (with protocol=0).
  2. Implement restriction of SCTP bind/connect via setsockopt(3p). This can be done by adding a hook on security_sctp_bind_connect (Cf. net/sctp/socket.c).
@l0kod
Copy link
Member

l0kod commented Sep 5, 2024

Well spotted! That restriction on SCTP sockets should be considered a bug because the LANDLOCK_ACCESS_NET_BIND_TCP (and the related documentation) is explicitly about TCP, not SCTP. We need a fix and related tests for that, and they should be backported. Do you want to work on that? It might be easier to first merge this fix (because of the backport) and then merge your work on socket creation (I'll review it too).

Complementary to this fix, it would be nice to be able to control SCTP sockets with a new LANDLOCK_ACCESS_NET_BIND_SCTP that would also handle binding via setsockopt(), but should come with another patch series.

@l0kod l0kod mentioned this issue Sep 5, 2024
Closed
@l0kod l0kod added the bug Something isn't working label Sep 5, 2024
@sm1ling-knight
Copy link
Author

Hello Mickaël!

I'd like to implement dedicated patch-fix. Please assign this issue to me.

@l0kod l0kod changed the title Restrict SCTP bind/connect via setsockopt Fix SCTP bind/connect mishandling Sep 9, 2024
@l0kod l0kod added this to Landlock Sep 9, 2024
@l0kod l0kod moved this to Ready in Landlock Sep 9, 2024
@sm1ling-knight sm1ling-knight mentioned this issue Sep 13, 2024
Open
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Oct 3, 2024
@sm1ling-knight @intel-lab-lkp
landlock: Fix non-TCP sockets restriction
cec023f 
Do not check TCP access right if socket protocol is not IPPROTO_TCP.
LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
should not restrict bind(2) and connect(2) for non-TCP protocols
(SCTP, MPTCP, SMC).

Closes: landlock-lsm#40
Fixes: fff69fb ("landlock: Support network rules with TCP bind and connect")
Signed-off-by: Mikhail Ivanov <[email protected]>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Oct 17, 2024
@sm1ling-knight @intel-lab-lkp
landlock: Fix non-TCP sockets restriction
dad5495 
Do not check TCP access right if socket protocol is not IPPROTO_TCP.
LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
should not restrict bind(2) and connect(2) for non-TCP protocols
(SCTP, MPTCP, SMC).

sk_is_tcp() is used for this to check address family of the socket
before doing INET-specific address length validation. This is required
for error consistency.

Closes: landlock-lsm#40
Fixes: fff69fb ("landlock: Support network rules with TCP bind and connect")
Signed-off-by: Mikhail Ivanov <[email protected]>
l0kod pushed a commit that referenced this issue Dec 16, 2024
@gregkh
arm64:uprobe fix the uprobe SWBP_INSN in big-endian
7f1ef59 
[ Upstream commit 60f07e2 ]

We use uprobe in aarch64_be, which we found the tracee task would exit
due to SIGILL when we enable the uprobe trace.
We can see the replace inst from uprobe is not correct in aarch big-endian.
As in Armv8-A, instruction fetches are always treated as little-endian,
we should treat the UPROBE_SWBP_INSN as little-endian。

The test case is as following。
bash-4.4# ./mqueue_test_aarchbe 1 1 2 1 10 > /dev/null &
bash-4.4# cd /sys/kernel/debug/tracing/
bash-4.4# echo 'p:test /mqueue_test_aarchbe:0xc30 %x0 %x1' > uprobe_events
bash-4.4# echo 1 > events/uprobes/enable
bash-4.4#
bash-4.4# ps
  PID TTY          TIME CMD
  140 ?        00:00:01 bash
  237 ?        00:00:00 ps
[1]+  Illegal instruction     ./mqueue_test_aarchbe 1 1 2 1 100 > /dev/null

which we debug use gdb as following:

bash-4.4# gdb attach 155
(gdb) disassemble send
Dump of assembler code for function send:
   0x0000000000400c30 <+0>:     .inst   0xa00020d4 ; undefined
   0x0000000000400c34 <+4>:     mov     x29, sp
   0x0000000000400c38 <+8>:     str     w0, [sp, #28]
   0x0000000000400c3c <+12>:    strb    w1, [sp, #27]
   0x0000000000400c40 <+16>:    str     xzr, [sp, #40]
   0x0000000000400c44 <+20>:    str     xzr, [sp, torvalds#48]
   0x0000000000400c48 <+24>:    add     x0, sp, #0x1b
   0x0000000000400c4c <+28>:    mov     w3, #0x0                 // #0
   0x0000000000400c50 <+32>:    mov     x2, #0x1                 // #1
   0x0000000000400c54 <+36>:    mov     x1, x0
   0x0000000000400c58 <+40>:    ldr     w0, [sp, #28]
   0x0000000000400c5c <+44>:    bl      0x405e10 <mq_send>
   0x0000000000400c60 <+48>:    str     w0, [sp, torvalds#60]
   0x0000000000400c64 <+52>:    ldr     w0, [sp, torvalds#60]
   0x0000000000400c68 <+56>:    ldp     x29, x30, [sp], torvalds#64
   0x0000000000400c6c <+60>:    ret
End of assembler dump.
(gdb) info b
No breakpoints or watchpoints.
(gdb) c
Continuing.

Program received signal SIGILL, Illegal instruction.
0x0000000000400c30 in send ()
(gdb) x/10x 0x400c30
0x400c30 <send>:    0xd42000a0   0xfd030091      0xe01f00b9      0xe16f0039
0x400c40 <send+16>: 0xff1700f9   0xff1b00f9      0xe06f0091      0x03008052
0x400c50 <send+32>: 0x220080d2   0xe10300aa
(gdb) disassemble 0x400c30
Dump of assembler code for function send:
=> 0x0000000000400c30 <+0>:     .inst   0xa00020d4 ; undefined
   0x0000000000400c34 <+4>:     mov     x29, sp
   0x0000000000400c38 <+8>:     str     w0, [sp, #28]
   0x0000000000400c3c <+12>:    strb    w1, [sp, #27]
   0x0000000000400c40 <+16>:    str     xzr, [sp, #40]

Signed-off-by: junhua huang <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Will Deacon <[email protected]>
Stable-dep-of: 13f8f1e ("arm64: probes: Fix uprobes for big-endian kernels")
Signed-off-by: Sasha Levin <[email protected]>
l0kod pushed a commit that referenced this issue Dec 16, 2024
@gregkh
arm64:uprobe fix the uprobe SWBP_INSN in big-endian
acfb32d 
[ Upstream commit 60f07e2 ]

We use uprobe in aarch64_be, which we found the tracee task would exit
due to SIGILL when we enable the uprobe trace.
We can see the replace inst from uprobe is not correct in aarch big-endian.
As in Armv8-A, instruction fetches are always treated as little-endian,
we should treat the UPROBE_SWBP_INSN as little-endian。

The test case is as following。
bash-4.4# ./mqueue_test_aarchbe 1 1 2 1 10 > /dev/null &
bash-4.4# cd /sys/kernel/debug/tracing/
bash-4.4# echo 'p:test /mqueue_test_aarchbe:0xc30 %x0 %x1' > uprobe_events
bash-4.4# echo 1 > events/uprobes/enable
bash-4.4#
bash-4.4# ps
  PID TTY          TIME CMD
  140 ?        00:00:01 bash
  237 ?        00:00:00 ps
[1]+  Illegal instruction     ./mqueue_test_aarchbe 1 1 2 1 100 > /dev/null

which we debug use gdb as following:

bash-4.4# gdb attach 155
(gdb) disassemble send
Dump of assembler code for function send:
   0x0000000000400c30 <+0>:     .inst   0xa00020d4 ; undefined
   0x0000000000400c34 <+4>:     mov     x29, sp
   0x0000000000400c38 <+8>:     str     w0, [sp, #28]
   0x0000000000400c3c <+12>:    strb    w1, [sp, #27]
   0x0000000000400c40 <+16>:    str     xzr, [sp, #40]
   0x0000000000400c44 <+20>:    str     xzr, [sp, torvalds#48]
   0x0000000000400c48 <+24>:    add     x0, sp, #0x1b
   0x0000000000400c4c <+28>:    mov     w3, #0x0                 // #0
   0x0000000000400c50 <+32>:    mov     x2, #0x1                 // #1
   0x0000000000400c54 <+36>:    mov     x1, x0
   0x0000000000400c58 <+40>:    ldr     w0, [sp, #28]
   0x0000000000400c5c <+44>:    bl      0x405e10 <mq_send>
   0x0000000000400c60 <+48>:    str     w0, [sp, torvalds#60]
   0x0000000000400c64 <+52>:    ldr     w0, [sp, torvalds#60]
   0x0000000000400c68 <+56>:    ldp     x29, x30, [sp], torvalds#64
   0x0000000000400c6c <+60>:    ret
End of assembler dump.
(gdb) info b
No breakpoints or watchpoints.
(gdb) c
Continuing.

Program received signal SIGILL, Illegal instruction.
0x0000000000400c30 in send ()
(gdb) x/10x 0x400c30
0x400c30 <send>:    0xd42000a0   0xfd030091      0xe01f00b9      0xe16f0039
0x400c40 <send+16>: 0xff1700f9   0xff1b00f9      0xe06f0091      0x03008052
0x400c50 <send+32>: 0x220080d2   0xe10300aa
(gdb) disassemble 0x400c30
Dump of assembler code for function send:
=> 0x0000000000400c30 <+0>:     .inst   0xa00020d4 ; undefined
   0x0000000000400c34 <+4>:     mov     x29, sp
   0x0000000000400c38 <+8>:     str     w0, [sp, #28]
   0x0000000000400c3c <+12>:    strb    w1, [sp, #27]
   0x0000000000400c40 <+16>:    str     xzr, [sp, #40]

Signed-off-by: junhua huang <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Will Deacon <[email protected]>
Stable-dep-of: 13f8f1e ("arm64: probes: Fix uprobes for big-endian kernels")
Signed-off-by: Sasha Levin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sm1ling-knight sm1ling-knight

Labels
bug Something isn't working
Projects
Landlock
Status: Ready
Milestone
No milestone
Development

No branches or pull requests
2 participants
@l0kod @sm1ling-knight