netdev - Re: [PATCH] RDS: verify the underlying transport exists before creating a connection

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <55EA092C.9000509@oracle.com>
Date:	Fri, 4 Sep 2015 14:12:12 -0700
From:	"santosh.shilimkar@...cle.com" <santosh.shilimkar@...cle.com>
To:	Sasha Levin <sasha.levin@...cle.com>, chien.yen@...cle.com,
	davem@...emloft.net
Cc:	rds-devel@....oracle.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] RDS: verify the underlying transport exists before
 creating a connection


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@...cle.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@...cle.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html