* [PATCH] RDS: verify the underlying transport exists before creating a connection @ 2015-09-04 16:43 Sasha Levin 2015-09-04 17:32 ` santosh shilimkar 2015-09-04 20:24 ` Sergei Shtylyov 0 siblings, 2 replies; 5+ messages in thread From: Sasha Levin @ 2015-09-04 16:43 UTC (permalink / raw) To: chien.yen, davem; +Cc: rds-devel, netdev, linux-kernel, Sasha Levin There was no verification that an underlying transport exists when creating a connection, this would cause dereferencing a NULL ptr. Signed-off-by: Sasha Levin <sasha.levin@oracle.com> --- net/rds/connection.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/net/rds/connection.c b/net/rds/connection.c index a50e652..0218d81 100644 --- a/net/rds/connection.c +++ b/net/rds/connection.c @@ -189,6 +189,12 @@ new_conn: } } + if (trans == NULL) { + kmem_cache_free(rds_conn_slab, conn); + conn = ERR_PTR(-ENODEV); + goto out; + } + conn->c_trans = trans; ret = trans->conn_alloc(conn, gfp); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] RDS: verify the underlying transport exists before creating a connection 2015-09-04 16:43 [PATCH] RDS: verify the underlying transport exists before creating a connection Sasha Levin @ 2015-09-04 17:32 ` santosh shilimkar 2015-09-04 19:44 ` Sasha Levin 2015-09-04 20:24 ` Sergei Shtylyov 1 sibling, 1 reply; 5+ messages in thread From: santosh shilimkar @ 2015-09-04 17:32 UTC (permalink / raw) To: Sasha Levin, chien.yen, davem; +Cc: rds-devel, netdev, linux-kernel Sasha, On 9/4/2015 9:43 AM, Sasha Levin wrote: > There was no verification that an underlying transport exists when creating > a connection, this would cause dereferencing a NULL ptr. > > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > --- > net/rds/connection.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/net/rds/connection.c b/net/rds/connection.c > index a50e652..0218d81 100644 > --- a/net/rds/connection.c > +++ b/net/rds/connection.c > @@ -189,6 +189,12 @@ new_conn: > } > } > > + if (trans == NULL) { > + kmem_cache_free(rds_conn_slab, conn); > + conn = ERR_PTR(-ENODEV); > + goto out; > + } > + Did you see the NULL oops in any tests ? The reason am asking this because callers of '__rds_conn_create()' are not passing the trans as null so that leaves with only the loopback case. In that case as well, rds_loop_transport is never going to be null. The check is good but am curious whether we have a case which will hit this scenario. Regards, Santosh ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RDS: verify the underlying transport exists before creating a connection 2015-09-04 17:32 ` santosh shilimkar @ 2015-09-04 19:44 ` Sasha Levin 2015-09-04 21:12 ` santosh.shilimkar 0 siblings, 1 reply; 5+ messages in thread From: Sasha Levin @ 2015-09-04 19:44 UTC (permalink / raw) To: santosh shilimkar, chien.yen, davem; +Cc: rds-devel, netdev, linux-kernel On 09/04/2015 01:32 PM, santosh shilimkar wrote: > Sasha, > > On 9/4/2015 9:43 AM, Sasha Levin wrote: >> There was no verification that an underlying transport exists when creating >> a connection, this would cause dereferencing a NULL ptr. >> >> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> >> --- >> net/rds/connection.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/net/rds/connection.c b/net/rds/connection.c >> index a50e652..0218d81 100644 >> --- a/net/rds/connection.c >> +++ b/net/rds/connection.c >> @@ -189,6 +189,12 @@ new_conn: >> } >> } >> >> + if (trans == NULL) { >> + kmem_cache_free(rds_conn_slab, conn); >> + conn = ERR_PTR(-ENODEV); >> + goto out; >> + } >> + > > Did you see the NULL oops in any tests ? The reason > am asking this because callers of '__rds_conn_create()' > are not passing the trans as null so that leaves with > only the loopback case. In that case as well, > rds_loop_transport is never going to be null. > > The check is good but am curious whether we have a > case which will hit this scenario. This is the trace I have: [135546.047719] kasan: GPF could be caused by NULL-ptr deref or user memory accessgeneral protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN [135546.051270] Modules linked in: [135546.051781] CPU: 4 PID: 15650 Comm: trinity-c4 Not tainted 4.2.0-next-20150902-sasha-00041-gbaa1222-dirty #2527 [135546.053217] task: ffff8800835bc000 ti: ffff8800bc708000 task.ti: ffff8800bc708000 [135546.054291] RIP: __rds_conn_create (net/rds/connection.c:194) [135546.055666] RSP: 0018:ffff8800bc70fab0 EFLAGS: 00010202 [135546.056457] RAX: dffffc0000000000 RBX: 0000000000000f2c RCX: ffff8800835bc000 [135546.057494] RDX: 0000000000000007 RSI: ffff8800835bccd8 RDI: 0000000000000038 [135546.058530] RBP: ffff8800bc70fb18 R08: 0000000000000001 R09: 0000000000000000 [135546.059556] R10: ffffed014d7a3a23 R11: ffffed014d7a3a21 R12: 0000000000000000 [135546.060614] R13: 0000000000000001 R14: ffff8801ec3d0000 R15: 0000000000000000 [135546.061668] FS: 00007faad4ffb700(0000) GS:ffff880252000000(0000) knlGS:0000000000000000 [135546.062836] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b [135546.063682] CR2: 000000000000846a CR3: 000000009d137000 CR4: 00000000000006a0 [135546.064723] Stack: [135546.065048] ffffffffafe2055c ffffffffafe23fc1 ffffed00493097bf ffff8801ec3d0008 [135546.066247] 0000000000000000 00000000000000d0 0000000000000000 ac194a24c0586342 [135546.067438] 1ffff100178e1f78 ffff880320581b00 ffff8800bc70fdd0 ffff880320581b00 [135546.068629] Call Trace: [135546.069028] ? __rds_conn_create (include/linux/rcupdate.h:856 net/rds/connection.c:134) [135546.069989] ? rds_message_copy_from_user (net/rds/message.c:298) [135546.071021] rds_conn_create_outgoing (net/rds/connection.c:278) [135546.071981] rds_sendmsg (net/rds/send.c:1058) [135546.072858] ? perf_trace_lock (include/trace/events/lock.h:38) [135546.073744] ? lockdep_init (kernel/locking/lockdep.c:3298) [135546.074577] ? rds_send_drop_to (net/rds/send.c:976) [135546.075508] ? __might_fault (./arch/x86/include/asm/current.h:14 mm/memory.c:3795) [135546.076349] ? __might_fault (mm/memory.c:3795) [135546.077179] ? rds_send_drop_to (net/rds/send.c:976) [135546.078114] sock_sendmsg (net/socket.c:611 net/socket.c:620) [135546.078856] SYSC_sendto (net/socket.c:1657) [135546.079596] ? SYSC_connect (net/socket.c:1628) [135546.080510] ? trace_dump_stack (kernel/trace/trace.c:1926) [135546.081397] ? ring_buffer_unlock_commit (kernel/trace/ring_buffer.c:2479 kernel/trace/ring_buffer.c:2558 kernel/trace/ring_buffer.c:2674) [135546.082390] ? trace_buffer_unlock_commit (kernel/trace/trace.c:1749) [135546.083410] ? trace_event_raw_event_sys_enter (include/trace/events/syscalls.h:16) [135546.084481] ? do_audit_syscall_entry (include/trace/events/syscalls.h:16) [135546.085438] ? trace_buffer_unlock_commit (kernel/trace/trace.c:1749) [135546.085515] rds_ib_laddr_check(): addr 36.74.25.172 ret -99 node type -1 [135546.087403] ? lock_is_held (kernel/locking/lockdep.c:3666) [135546.088255] ? rcu_read_lock_sched_held (kernel/rcu/update.c:109) [135546.089273] SyS_sendto (net/socket.c:1625) [135546.090112] tracesys_phase2 (arch/x86/entry/entry_64.S:273) [135546.090934] Code: 03 80 3c 02 00 0f 85 86 0b 00 00 48 8b 45 b8 48 8d 78 38 49 89 86 d8 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 70 0b 00 00 48 8b 45 b8 4c 89 f7 8b 75 c0 ff All code ======== 0: 03 80 3c 02 00 0f add 0xf00023c(%rax),%eax 6: 85 86 0b 00 00 48 test %eax,0x4800000b(%rsi) c: 8b 45 b8 mov -0x48(%rbp),%eax f: 48 8d 78 38 lea 0x38(%rax),%rdi 13: 49 89 86 d8 00 00 00 mov %rax,0xd8(%r14) 1a: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax 21: fc ff df 24: 48 89 fa mov %rdi,%rdx 27: 48 c1 ea 03 shr $0x3,%rdx 2b:* 80 3c 02 00 cmpb $0x0,(%rdx,%rax,1) <-- trapping instruction 2f: 0f 85 70 0b 00 00 jne 0xba5 35: 48 8b 45 b8 mov -0x48(%rbp),%rax 39: 4c 89 f7 mov %r14,%rdi 3c: 8b 75 c0 mov -0x40(%rbp),%esi 3f: ff 00 incl (%rax) Code starting with the faulting instruction =========================================== 0: 80 3c 02 00 cmpb $0x0,(%rdx,%rax,1) 4: 0f 85 70 0b 00 00 jne 0xb7a a: 48 8b 45 b8 mov -0x48(%rbp),%rax e: 4c 89 f7 mov %r14,%rdi 11: 8b 75 c0 mov -0x40(%rbp),%esi 14: ff 00 incl (%rax) [135546.095824] RIP __rds_conn_create (net/rds/connection.c:194) [135546.096728] RSP <ffff8800bc70fab0> Thanks, Sasha ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RDS: verify the underlying transport exists before creating a connection 2015-09-04 19:44 ` Sasha Levin @ 2015-09-04 21:12 ` santosh.shilimkar 0 siblings, 0 replies; 5+ messages in thread From: santosh.shilimkar @ 2015-09-04 21:12 UTC (permalink / raw) To: Sasha Levin, chien.yen, davem; +Cc: rds-devel, netdev, linux-kernel On 9/4/15 12:44 PM, Sasha Levin wrote: > On 09/04/2015 01:32 PM, santosh shilimkar wrote: >> Sasha, >> >> On 9/4/2015 9:43 AM, Sasha Levin wrote: >>> There was no verification that an underlying transport exists when creating >>> a connection, this would cause dereferencing a NULL ptr. >>> >>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> >>> --- >>> net/rds/connection.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/net/rds/connection.c b/net/rds/connection.c >>> index a50e652..0218d81 100644 >>> --- a/net/rds/connection.c >>> +++ b/net/rds/connection.c >>> @@ -189,6 +189,12 @@ new_conn: >>> } >>> } >>> >>> + if (trans == NULL) { >>> + kmem_cache_free(rds_conn_slab, conn); >>> + conn = ERR_PTR(-ENODEV); >>> + goto out; >>> + } >>> + >> >> Did you see the NULL oops in any tests ? The reason >> am asking this because callers of '__rds_conn_create()' >> are not passing the trans as null so that leaves with >> only the loopback case. In that case as well, >> rds_loop_transport is never going to be null. >> >> The check is good but am curious whether we have a >> case which will hit this scenario. > > This is the trace I have: > Thanks. > > [135546.047719] kasan: GPF could be caused by NULL-ptr deref or user memory accessgeneral protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN > [135546.051270] Modules linked in: > [135546.051781] CPU: 4 PID: 15650 Comm: trinity-c4 Not tainted 4.2.0-next-20150902-sasha-00041-gbaa1222-dirty #2527 > [135546.053217] task: ffff8800835bc000 ti: ffff8800bc708000 task.ti: ffff8800bc708000 > [135546.054291] RIP: __rds_conn_create (net/rds/connection.c:194) > [135546.055666] RSP: 0018:ffff8800bc70fab0 EFLAGS: 00010202 > [135546.056457] RAX: dffffc0000000000 RBX: 0000000000000f2c RCX: ffff8800835bc000 > [135546.057494] RDX: 0000000000000007 RSI: ffff8800835bccd8 RDI: 0000000000000038 > [135546.058530] RBP: ffff8800bc70fb18 R08: 0000000000000001 R09: 0000000000000000 > [135546.059556] R10: ffffed014d7a3a23 R11: ffffed014d7a3a21 R12: 0000000000000000 > [135546.060614] R13: 0000000000000001 R14: ffff8801ec3d0000 R15: 0000000000000000 > [135546.061668] FS: 00007faad4ffb700(0000) GS:ffff880252000000(0000) knlGS:0000000000000000 > [135546.062836] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b > [135546.063682] CR2: 000000000000846a CR3: 000000009d137000 CR4: 00000000000006a0 > [135546.064723] Stack: > [135546.065048] ffffffffafe2055c ffffffffafe23fc1 ffffed00493097bf ffff8801ec3d0008 > [135546.066247] 0000000000000000 00000000000000d0 0000000000000000 ac194a24c0586342 > [135546.067438] 1ffff100178e1f78 ffff880320581b00 ffff8800bc70fdd0 ffff880320581b00 > [135546.068629] Call Trace: > [135546.069028] ? __rds_conn_create (include/linux/rcupdate.h:856 net/rds/connection.c:134) > [135546.069989] ? rds_message_copy_from_user (net/rds/message.c:298) > [135546.071021] rds_conn_create_outgoing (net/rds/connection.c:278) > [135546.071981] rds_sendmsg (net/rds/send.c:1058) > [135546.072858] ? perf_trace_lock (include/trace/events/lock.h:38) > [135546.073744] ? lockdep_init (kernel/locking/lockdep.c:3298) > [135546.074577] ? rds_send_drop_to (net/rds/send.c:976) > [135546.075508] ? __might_fault (./arch/x86/include/asm/current.h:14 mm/memory.c:3795) > [135546.076349] ? __might_fault (mm/memory.c:3795) > [135546.077179] ? rds_send_drop_to (net/rds/send.c:976) > [135546.078114] sock_sendmsg (net/socket.c:611 net/socket.c:620) > [135546.078856] SYSC_sendto (net/socket.c:1657) > [135546.079596] ? SYSC_connect (net/socket.c:1628) > [135546.080510] ? trace_dump_stack (kernel/trace/trace.c:1926) > [135546.081397] ? ring_buffer_unlock_commit (kernel/trace/ring_buffer.c:2479 kernel/trace/ring_buffer.c:2558 kernel/trace/ring_buffer.c:2674) > [135546.082390] ? trace_buffer_unlock_commit (kernel/trace/trace.c:1749) > [135546.083410] ? trace_event_raw_event_sys_enter (include/trace/events/syscalls.h:16) > [135546.084481] ? do_audit_syscall_entry (include/trace/events/syscalls.h:16) > [135546.085438] ? trace_buffer_unlock_commit (kernel/trace/trace.c:1749) > [135546.085515] rds_ib_laddr_check(): addr 36.74.25.172 ret -99 node type -1 So rds_ib_laddr_check() failed means either the app isn't doing bind() before sendmsg() or ignoring the error returned by bind() and going ahead and issuing the send. Anyway, we can't control the application so having this hole plugged is good. Please include the back trace in the commit log. With that update, Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RDS: verify the underlying transport exists before creating a connection 2015-09-04 16:43 [PATCH] RDS: verify the underlying transport exists before creating a connection Sasha Levin 2015-09-04 17:32 ` santosh shilimkar @ 2015-09-04 20:24 ` Sergei Shtylyov 1 sibling, 0 replies; 5+ messages in thread From: Sergei Shtylyov @ 2015-09-04 20:24 UTC (permalink / raw) To: Sasha Levin, chien.yen, davem; +Cc: rds-devel, netdev, linux-kernel Hello. On 09/04/2015 07:43 PM, Sasha Levin wrote: > There was no verification that an underlying transport exists when creating > a connection, this would cause dereferencing a NULL ptr. > > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > --- > net/rds/connection.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/net/rds/connection.c b/net/rds/connection.c > index a50e652..0218d81 100644 > --- a/net/rds/connection.c > +++ b/net/rds/connection.c > @@ -189,6 +189,12 @@ new_conn: > } > } > > + if (trans == NULL) { Please use !trans instead, it's the form preferred by the networking code. [...] MBR, Sergei ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-09-04 21:12 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-04 16:43 [PATCH] RDS: verify the underlying transport exists before creating a connection Sasha Levin 2015-09-04 17:32 ` santosh shilimkar 2015-09-04 19:44 ` Sasha Levin 2015-09-04 21:12 ` santosh.shilimkar 2015-09-04 20:24 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).