Re: [PATCH] riscv: Support RANDOMIZE_KSTACK_OFFSET

2023-11-01 Thread Damien Le Moal 
On 11/1/23 15:44, Song Shuai wrote:
> Inspired from arm64's implement -- commit 70918779aec9
> ("arm64: entry: Enable random_kstack_offset support")
> 
> Add support of kernel stack offset randomization while handling syscall,
> the offset is defaultly limited by KSTACK_OFFSET_MAX() (i.e. 10 bits).
> 
> In order to avoid trigger stack canaries (due to __builtin_alloca) and
> slowing down the entry path, use __no_stack_protector attribute to
> disable stack protector for do_trap_ecall_u() at the function level.
> 
> Signed-off-by: Song Shuai 
> ---
> Testing with randomize_kstack_offset=y cmdline, lkdtm/stack-entropy.sh
> showed appropriate stack offset instead of zero.
> ---
>  arch/riscv/Kconfig|  1 +
>  arch/riscv/kernel/traps.c | 18 +-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d607ab0f7c6d..0e843de33f0c 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -100,6 +100,7 @@ config RISCV
>   select HAVE_ARCH_KGDB_QXFER_PKT
>   select HAVE_ARCH_MMAP_RND_BITS if MMU
>   select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
> + select HAVE_ARCH_RANDOMIZE_KSTACK_OFFSET
>   select HAVE_ARCH_SECCOMP_FILTER
>   select HAVE_ARCH_THREAD_STRUCT_WHITELIST
>   select HAVE_ARCH_TRACEHOOK
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index 19807c4d3805..3f869b2d47c3 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -6,6 +6,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -296,9 +297,11 @@ asmlinkage __visible __trap_section void 
> do_trap_break(struct pt_regs *regs)
>   }
>  }
>  
> -asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs 
> *regs)
> +asmlinkage __visible __trap_section  __no_stack_protector
> +void do_trap_ecall_u(struct pt_regs *regs)
>  {
>   if (user_mode(regs)) {
> +

White line change.

>   long syscall = regs->a7;
>  
>   regs->epc += 4;
> @@ -308,10 +311,23 @@ asmlinkage __visible __trap_section void 
> do_trap_ecall_u(struct pt_regs *regs)
>  
>   syscall = syscall_enter_from_user_mode(regs, syscall);
>  
> + add_random_kstack_offset();
> +
>   if (syscall >= 0 && syscall < NR_syscalls)
>   syscall_handler(regs, syscall);
>   else if (syscall != -1)
>   regs->a0 = -ENOSYS;
> + /*
> +  * Ultimately, this value will get limited by 
> KSTACK_OFFSET_MAX(),
> +  * so the maximum stack offset is 1k bytes (10 bits).
> +  *
> +  * The actual entropy will be further reduced by the compiler 
> when
> +  * applying stack alignment constraints: 16-byte (i.e. 4-bit) 
> aligned
> +  * for RV32I or RV64I.
> +  *
> +  * The resulting 6 bits of entropy is seen in SP[9:4].
> +  */
> + choose_random_kstack_offset(get_random_u16());
>  
>   syscall_exit_to_user_mode(regs);
>   } else {

-- 
Damien Le Moal
Western Digital Research

[PATCH V2] usb: musb: Check requset->buf before use to avoid crash issue

2023-11-01 Thread Xingxing Luo 
When connecting USB to PC, there is a very low probability of kernel
crash. The reason is that in ep0_txstate(), the buf member of struct
usb_request used may be a null pointer. Therefore, it needs to
determine whether it is null before using it.

[ 4888.071462][T597@C0] Call trace:
[ 4888.071467][T597@C0]  musb_default_write_fifo+0xa0/0x1ac [musb_hdrc]
[ 4888.087190][T597@C0]  musb_write_fifo+0x3c/0x90 [musb_hdrc]
[ 4888.099826][T597@C0]  ep0_txstate+0x78/0x218 [musb_hdrc]
[ 4888.153918][T597@C0]  musb_g_ep0_irq+0x3c4/0xe10 [musb_hdrc]
[ 4888.159663][T597@C0]  musb_interrupt+0xab4/0xf1c [musb_hdrc]
[ 4888.165391][T597@C0]  sprd_musb_interrupt+0x1e4/0x484 [musb_sprd]
[ 4888.171447][T597@C0]  __handle_irq_event_percpu+0xd8/0x2f8
[ 4888.176901][T597@C0]  handle_irq_event+0x70/0xe4
[ 4888.181487][T597@C0]  handle_fasteoi_irq+0x15c/0x230
[ 4888.186420][T597@C0]  handle_domain_irq+0x88/0xfc
[ 4888.191090][T597@C0]  gic_handle_irq+0x60/0x138
[ 4888.195591][T597@C0]  call_on_irq_stack+0x40/0x70
[ 4888.200263][T597@C0]  do_interrupt_handler+0x50/0xac
[ 4888.205196][T597@C0]  el1_interrupt+0x34/0x64
[ 4888.209524][T597@C0]  el1h_64_irq_handler+0x1c/0x2c
[ 4888.214370][T597@C0]  el1h_64_irq+0x7c/0x80
[ 4888.218525][T597@C0]  __check_heap_object+0x1ac/0x1fc
[ 4888.223544][T597@C0]  __check_object_size+0x10c/0x20c
[ 4888.228563][T597@C0]  simple_copy_to_iter+0x40/0x74
[ 4888.233410][T597@C0]  __skb_datagram_iter+0xa0/0x310
[ 4888.238343][T597@C0]  skb_copy_datagram_iter+0x44/0x110
[ 4888.243535][T597@C0]  netlink_recvmsg+0xdc/0x364
[ 4888.248123][T597@C0]  sys_recvmsg.llvm.16749613423860851707+0x358/0x6c0
[ 4888.255045][T597@C0]  ___sys_recvmsg+0xe0/0x1dc
[ 4888.259544][T597@C0]  __arm64_sys_recvmsg+0xc4/0x10c
[ 4888.264478][T597@C0]  invoke_syscall+0x6c/0x15c
[ 4888.268976][T597@C0]  el0_svc_common.llvm.12373701176611417606+0xd4/0x120
[ 4888.275726][T597@C0]  do_el0_svc+0x34/0xac
[ 4888.279795][T597@C0]  el0_svc+0x28/0x90
[ 4888.283603][T597@C0]  el0t_64_sync_handler+0x88/0xec
[ 4888.288548][T597@C0]  el0t_64_sync+0x1b4/0x1b8
[ 4888.292956][T597@C0] Code: 540002c3 53027ea8 aa1303e9 71000508 (b840452a)
[ 4888.299789][T597@C0] ---[ end trace 14a301b7253e83cc ]---

Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")
Signed-off-by: Xingxing Luo 
---
v1 -> v2: - Fixed a spelling error
  - Add the fixed commit id

 drivers/usb/musb/musb_gadget_ep0.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/musb/musb_gadget_ep0.c 
b/drivers/usb/musb/musb_gadget_ep0.c
index 6d7336727388..19eb7a5e1fdc 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -531,6 +531,11 @@ static void ep0_txstate(struct musb *musb)
 
request = &req->request;
 
+   if (!request->buf) {
+   musb_dbg(musb, "request->buf is NULL");
+   return;
+   }
+
/* load the data */
fifo_src = (u8 *) request->buf + request->actual;
fifo_count = min((unsigned) MUSB_EP0_FIFOSIZE,
-- 
2.17.1

Re: [PATCH V2] usb: musb: Check requset->buf before use to avoid crash issue

2023-11-01 Thread Greg KH 
On Wed, Nov 01, 2023 at 03:14:21PM +0800, Xingxing Luo wrote:
> When connecting USB to PC, there is a very low probability of kernel
> crash. The reason is that in ep0_txstate(), the buf member of struct
> usb_request used may be a null pointer. Therefore, it needs to
> determine whether it is null before using it.
> 
> [ 4888.071462][T597@C0] Call trace:
> [ 4888.071467][T597@C0]  musb_default_write_fifo+0xa0/0x1ac [musb_hdrc]
> [ 4888.087190][T597@C0]  musb_write_fifo+0x3c/0x90 [musb_hdrc]
> [ 4888.099826][T597@C0]  ep0_txstate+0x78/0x218 [musb_hdrc]
> [ 4888.153918][T597@C0]  musb_g_ep0_irq+0x3c4/0xe10 [musb_hdrc]
> [ 4888.159663][T597@C0]  musb_interrupt+0xab4/0xf1c [musb_hdrc]
> [ 4888.165391][T597@C0]  sprd_musb_interrupt+0x1e4/0x484 [musb_sprd]
> [ 4888.171447][T597@C0]  __handle_irq_event_percpu+0xd8/0x2f8
> [ 4888.176901][T597@C0]  handle_irq_event+0x70/0xe4
> [ 4888.181487][T597@C0]  handle_fasteoi_irq+0x15c/0x230
> [ 4888.186420][T597@C0]  handle_domain_irq+0x88/0xfc
> [ 4888.191090][T597@C0]  gic_handle_irq+0x60/0x138
> [ 4888.195591][T597@C0]  call_on_irq_stack+0x40/0x70
> [ 4888.200263][T597@C0]  do_interrupt_handler+0x50/0xac
> [ 4888.205196][T597@C0]  el1_interrupt+0x34/0x64
> [ 4888.209524][T597@C0]  el1h_64_irq_handler+0x1c/0x2c
> [ 4888.214370][T597@C0]  el1h_64_irq+0x7c/0x80
> [ 4888.218525][T597@C0]  __check_heap_object+0x1ac/0x1fc
> [ 4888.223544][T597@C0]  __check_object_size+0x10c/0x20c
> [ 4888.228563][T597@C0]  simple_copy_to_iter+0x40/0x74
> [ 4888.233410][T597@C0]  __skb_datagram_iter+0xa0/0x310
> [ 4888.238343][T597@C0]  skb_copy_datagram_iter+0x44/0x110
> [ 4888.243535][T597@C0]  netlink_recvmsg+0xdc/0x364
> [ 4888.248123][T597@C0]  sys_recvmsg.llvm.16749613423860851707+0x358/0x6c0
> [ 4888.255045][T597@C0]  ___sys_recvmsg+0xe0/0x1dc
> [ 4888.259544][T597@C0]  __arm64_sys_recvmsg+0xc4/0x10c
> [ 4888.264478][T597@C0]  invoke_syscall+0x6c/0x15c
> [ 4888.268976][T597@C0]  el0_svc_common.llvm.12373701176611417606+0xd4/0x120
> [ 4888.275726][T597@C0]  do_el0_svc+0x34/0xac
> [ 4888.279795][T597@C0]  el0_svc+0x28/0x90
> [ 4888.283603][T597@C0]  el0t_64_sync_handler+0x88/0xec
> [ 4888.288548][T597@C0]  el0t_64_sync+0x1b4/0x1b8
> [ 4888.292956][T597@C0] Code: 540002c3 53027ea8 aa1303e9 71000508 (b840452a)
> [ 4888.299789][T597@C0] ---[ end trace 14a301b7253e83cc ]---
> 
> Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")
> Signed-off-by: Xingxing Luo 
> ---
> v1 -> v2: - Fixed a spelling error
>   - Add the fixed commit id
> 
>  drivers/usb/musb/musb_gadget_ep0.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/musb/musb_gadget_ep0.c 
> b/drivers/usb/musb/musb_gadget_ep0.c
> index 6d7336727388..19eb7a5e1fdc 100644
> --- a/drivers/usb/musb/musb_gadget_ep0.c
> +++ b/drivers/usb/musb/musb_gadget_ep0.c
> @@ -531,6 +531,11 @@ static void ep0_txstate(struct musb *musb)
>  
>   request = &req->request;
>  
> + if (!request->buf) {
> + musb_dbg(musb, "request->buf is NULL");
> + return;
> + }
> +
>   /* load the data */
>   fifo_src = (u8 *) request->buf + request->actual;
>   fifo_count = min((unsigned) MUSB_EP0_FIFOSIZE,
> -- 
> 2.17.1
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH V2] usb: musb: Check requset->buf before use to avoid crash issue

2023-11-01 Thread Greg KH 
On Wed, Nov 01, 2023 at 03:14:21PM +0800, Xingxing Luo wrote:
> When connecting USB to PC, there is a very low probability of kernel
> crash. The reason is that in ep0_txstate(), the buf member of struct
> usb_request used may be a null pointer. Therefore, it needs to
> determine whether it is null before using it.
> 
> [ 4888.071462][T597@C0] Call trace:
> [ 4888.071467][T597@C0]  musb_default_write_fifo+0xa0/0x1ac [musb_hdrc]
> [ 4888.087190][T597@C0]  musb_write_fifo+0x3c/0x90 [musb_hdrc]
> [ 4888.099826][T597@C0]  ep0_txstate+0x78/0x218 [musb_hdrc]
> [ 4888.153918][T597@C0]  musb_g_ep0_irq+0x3c4/0xe10 [musb_hdrc]
> [ 4888.159663][T597@C0]  musb_interrupt+0xab4/0xf1c [musb_hdrc]
> [ 4888.165391][T597@C0]  sprd_musb_interrupt+0x1e4/0x484 [musb_sprd]
> [ 4888.171447][T597@C0]  __handle_irq_event_percpu+0xd8/0x2f8
> [ 4888.176901][T597@C0]  handle_irq_event+0x70/0xe4
> [ 4888.181487][T597@C0]  handle_fasteoi_irq+0x15c/0x230
> [ 4888.186420][T597@C0]  handle_domain_irq+0x88/0xfc
> [ 4888.191090][T597@C0]  gic_handle_irq+0x60/0x138
> [ 4888.195591][T597@C0]  call_on_irq_stack+0x40/0x70
> [ 4888.200263][T597@C0]  do_interrupt_handler+0x50/0xac
> [ 4888.205196][T597@C0]  el1_interrupt+0x34/0x64
> [ 4888.209524][T597@C0]  el1h_64_irq_handler+0x1c/0x2c
> [ 4888.214370][T597@C0]  el1h_64_irq+0x7c/0x80
> [ 4888.218525][T597@C0]  __check_heap_object+0x1ac/0x1fc
> [ 4888.223544][T597@C0]  __check_object_size+0x10c/0x20c
> [ 4888.228563][T597@C0]  simple_copy_to_iter+0x40/0x74
> [ 4888.233410][T597@C0]  __skb_datagram_iter+0xa0/0x310
> [ 4888.238343][T597@C0]  skb_copy_datagram_iter+0x44/0x110
> [ 4888.243535][T597@C0]  netlink_recvmsg+0xdc/0x364
> [ 4888.248123][T597@C0]  sys_recvmsg.llvm.16749613423860851707+0x358/0x6c0
> [ 4888.255045][T597@C0]  ___sys_recvmsg+0xe0/0x1dc
> [ 4888.259544][T597@C0]  __arm64_sys_recvmsg+0xc4/0x10c
> [ 4888.264478][T597@C0]  invoke_syscall+0x6c/0x15c
> [ 4888.268976][T597@C0]  el0_svc_common.llvm.12373701176611417606+0xd4/0x120
> [ 4888.275726][T597@C0]  do_el0_svc+0x34/0xac
> [ 4888.279795][T597@C0]  el0_svc+0x28/0x90
> [ 4888.283603][T597@C0]  el0t_64_sync_handler+0x88/0xec
> [ 4888.288548][T597@C0]  el0t_64_sync+0x1b4/0x1b8
> [ 4888.292956][T597@C0] Code: 540002c3 53027ea8 aa1303e9 71000508 (b840452a)
> [ 4888.299789][T597@C0] ---[ end trace 14a301b7253e83cc ]---
> 
> Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")
> Signed-off-by: Xingxing Luo 
> ---
> v1 -> v2: - Fixed a spelling error
>   - Add the fixed commit id
> 
>  drivers/usb/musb/musb_gadget_ep0.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/musb/musb_gadget_ep0.c 
> b/drivers/usb/musb/musb_gadget_ep0.c
> index 6d7336727388..19eb7a5e1fdc 100644
> --- a/drivers/usb/musb/musb_gadget_ep0.c
> +++ b/drivers/usb/musb/musb_gadget_ep0.c
> @@ -531,6 +531,11 @@ static void ep0_txstate(struct musb *musb)
>  
>   request = &req->request;
>  
> + if (!request->buf) {
> + musb_dbg(musb, "request->buf is NULL");

Why is this debug line needed?

> + return;

Shouldn't we be reporting an error here somehow?

And why has this issue never been seen before in this driver?  This is a
very old driver, with millions, if not billions, of working systems with
it.  What caused this to suddenly start happening?

thanks,

greg k-h

Re: [PATCH V2] usb: musb: Check requset->buf before use to avoid crash issue

2023-11-01 Thread Sergey Shtylyov 
Hello!

   You have have a typo in the subject: s/requset/request/...

MBR, Sergey

Re: Isolating abstract sockets

2023-11-01 Thread Mickaël Salaün 
On Tue, Oct 31, 2023 at 09:40:59PM +0100, Stefan Bavendiek wrote:
> On Tue, Oct 24, 2023 at 11:07:14AM -0500, Serge E. Hallyn wrote:
> > In 2005, before namespaces were upstreamed, I posted the 'bsdjail' LSM,
> > which briefly made it into the -mm kernel, but was eventually rejected as
> > being an abuse of the LSM interface for OS level virtualization :)
> > 
> > It's not 100% clear to me whether Stefan only wants isolation, or
> > wants something closer to virtualization.
> > 
> > Stefan, would an LSM allowing you to isolate certain processes from
> > some abstract unix socket paths (or by label, whatever0 suffice for you?
> >
> 
> My intention was to find a clean way to isolate abstract sockets in network
> applications without adding dependencies like LSMs. However the entire 
> approach
> of using namespaces for this is something I have mostly abandoned. LSMs like
> Apparmor and SELinux would work fine for process isolation when you can 
> control
> the target system, but for general deployment of sandboxed processes, I found 
> it
> to be significantly easier (and more effective) to build this into the
> application itself by using a multi process approach with seccomp (Basically 
> how
> OpenSSH did it)

I agree that for sandbox use cases embedding such security policy into
the application itself makes sense. Landlock works the same way as
seccomp but it sandboxes applications according to the kernel semantic
(e.g. process, socket). The LSM framework is just a kernel
implementation detail. ;)

Re: Isolating abstract sockets

2023-11-01 Thread Jann Horn 
On Wed, Nov 1, 2023 at 11:57 AM Mickaël Salaün  wrote:
> On Tue, Oct 31, 2023 at 09:40:59PM +0100, Stefan Bavendiek wrote:
> > On Tue, Oct 24, 2023 at 11:07:14AM -0500, Serge E. Hallyn wrote:
> > > In 2005, before namespaces were upstreamed, I posted the 'bsdjail' LSM,
> > > which briefly made it into the -mm kernel, but was eventually rejected as
> > > being an abuse of the LSM interface for OS level virtualization :)
> > >
> > > It's not 100% clear to me whether Stefan only wants isolation, or
> > > wants something closer to virtualization.
> > >
> > > Stefan, would an LSM allowing you to isolate certain processes from
> > > some abstract unix socket paths (or by label, whatever0 suffice for you?
> > >
> >
> > My intention was to find a clean way to isolate abstract sockets in network
> > applications without adding dependencies like LSMs. However the entire 
> > approach
> > of using namespaces for this is something I have mostly abandoned. LSMs like
> > Apparmor and SELinux would work fine for process isolation when you can 
> > control
> > the target system, but for general deployment of sandboxed processes, I 
> > found it
> > to be significantly easier (and more effective) to build this into the
> > application itself by using a multi process approach with seccomp 
> > (Basically how
> > OpenSSH did it)
>
> I agree that for sandbox use cases embedding such security policy into
> the application itself makes sense. Landlock works the same way as
> seccomp but it sandboxes applications according to the kernel semantic
> (e.g. process, socket). The LSM framework is just a kernel
> implementation detail. ;)

(Related, it might be nice if Landlock had a way to completely deny
access to abstract unix sockets, and a way to restrict filesystem unix
sockets with filesystem rules... LANDLOCK_ACCESS_FS_MAKE_SOCK exists
for restricting bind(), but I don't think there's an analogous
permission for connect().

Currently, when you try to sandbox an application with Landlock, you
have to use seccomp to completely block access to unix domain sockets,
or alternatively use something like the seccomp_unotify feature to
interactively filter connect() calls.

On the other hand, maybe such a feature would be a bit superfluous
when we have seccomp_unotify already... idk.)