[PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Michael J. Ruhl]

I was playing with the ibacm service and discovered an issue
the other day.

If no provider library is present (I removed libacmp.so, and the
provider keyword in the opts.cfg file is libacmp), when a resolve
request is posted, the kernel will crash with the following Oops:

BUG: unable to handle kernel NULL pointer dereference at (null)
IP:           (null)
PGD 10543f1067 P4D 10543f1067 PUD 1033f93067 PMD 0
Oops: 0010 [#1] SMP
Modules linked in: rpcrdma ib_isert iscsi_target_mod
 target_core_mod ib_iser libiscsi scsi_transport_iscsi ib_ipoib rdma_ucm ib_u
 ib_uverbs ib_umad rdma_cm ib_cm iw_cm dm_mirror dm_region_hash dm_log dm_mod
 dax sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass
 crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_si
 glue_helper cryptd hfi1 rdmavt iTCO_wdt iTCO_vendor_support ib_core mei_me
 lpc_ich pcspkr mei ioatdma sg shpchp i2c_i801 mfd_core wmi ipmi_si ipmi_devi
 ipmi_msghandler acpi_power_meter acpi_pad nfsd auth_rpcgss nfs_acl lockd gra
 sunrpc ip_tables ext4 mbcache jbd2 sd_mod mgag200 drm_kms_helper syscopyarea
 sysfillrect sysimgblt fb_sys_fops ttm igb ahci crc32c_intel ptp libahci
 pps_core drm dca libata i2c_algo_bit i2c_core
CPU: 54 PID: 9841 Comm: ibacm Tainted: G          I    4.14.0-rc2+ #6
Hardware name: Intel Corporation S2600WT2/S2600WT2, BIOS     SE5C610.86B.01.01.0008.021120151325 02/11/2015
task: ffff880855f42d00 task.stack: ffffc900246b4000
RIP: 0010:          (null)
RSP: 0018:ffffc900246b7bc8 EFLAGS: 00010246
RAX: ffffffff81dbe9e0 RBX: ffff881058bb1000 RCX: 0000000000000000
RDX: 0000000000001100 RSI: ffff881058bb1320 RDI: ffff881056362000
RBP: ffffc900246b7bf8 R08: 0000000000000ec0 R09: 0000000000001100
R10: ffff8810573a5000 R11: 0000000000000000 R12: ffff881056362000
R13: 0000000000000ec0 R14: ffff881058bb1320 R15: 0000000000000ec0
FS:  00007fe0ba5a38c0(0000) GS:ffff88105f080000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000001056f5d003 CR4: 00000000001606e0
Call Trace:
 ? netlink_dump+0x12c/0x290
 __netlink_dump_start+0x186/0x1f0
 rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
 rdma_nl_rcv+0xdc/0x130 [ib_core]
 netlink_unicast+0x181/0x240
 netlink_sendmsg+0x2c2/0x3b0
 sock_sendmsg+0x38/0x50
 SYSC_sendto+0x102/0x190
 ? __audit_syscall_entry+0xaf/0x100
 ? syscall_trace_enter+0x1d0/0x2b0
 ? __audit_syscall_exit+0x209/0x290
 SyS_sendto+0xe/0x10
 do_syscall_64+0x67/0x1b0
 entry_SYSCALL64_slow_path+0x25/0x25
RIP: 0033:0x7fe0b9db2a63
RSP: 002b:00007ffc55edc260 EFLAGS: 00000293 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 0000000000000010 RCX: 00007fe0b9db2a63
RDX: 0000000000000010 RSI: 00007ffc55edc280 RDI: 000000000000000d
RBP: 00007ffc55edc670 R08: 00007ffc55edc270 R09: 000000000000000c
R10: 0000000000000000 R11: 0000000000000293 R12: 00007ffc55edc280
R13: 000000000260b400 R14: 000000000000000d R15: 0000000000000001
Code:  Bad RIP value.
RIP:           (null) RSP: ffffc900246b7bc8
CR2: 0000000000000000
---[ end trace 8d67abcfd10ec209 ]---
Kernel panic - not syncing: Fatal exception
Kernel Offset: disabled
---[ end Kernel panic - not syncing: Fatal exception
------------[ cut here ]------------

The issue is that in rdma_nl_rcv_msg(), the check
'if (flags & NLM_F_DUMP)' is not completely correct.
    
NLM_F_DUMP is two bits NLM_F_ROOT | NLM_F_MATCH.

ibacm sends a RDMA_NL_LS response with the RDMA_NL_LS_F_ERR bit set
if an error occurs in the service (like no provider being available,
or ACM_STATUS_ENODATA, etc.).

NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
    
The current code thinks that it sees a NLM_F_DUMP flag and incorrectly calls
the .dump() callback.

The included patch is an atempt to fix this issue.  This patch fixes the
issue that I am seeing, but I am not sure how to test the messages for
RDMA_NL_RDMA_CM or RDMA_NL_IWCM (or any message that uses the
NLM_F_DUMP bits).

If anyone has some knowledge of these services, any extra testing would
be welcomed.

If the patch has no issues or comments, I will formally re-submit it
(through my usual channel Denny).

Thanks,

Mike


---

Michael J. Ruhl (1):
      RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag


 drivers/infiniband/core/netlink.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

-- 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Michael J. Ruhl]

From: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

rdma_nl_rcv_msg() checks to see if it should use the .dump() callback
or the .doit() callback.  The check is done with this check:

if (flags & NLM_F_DUMP) ...

The NLM_F_DUMP flag is two bits (NLM_F_ROOT | NLM_F_MATCH).

When an RDMA_NL_LS messages is received, the bit used for indicating
an error is the same bit as NLM_F_ROOT.

NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.

ibacm sends a response with the RDMA_NL_LS_F_ERR bit set if an error
occurs in the service.  The current code then misinterprets the
NLM_F_DUMP bit and trys to call the .dump() callback.

If the .dump() callback for the specified request is not available
(which is true for the RDMA_NL_LS messages) the following Oops occurs:

[ 4555.960256] BUG: unable to handle kernel NULL pointer dereference at
   (null)
[ 4555.969046] IP:           (null)
[ 4555.972664] PGD 10543f1067 P4D 10543f1067 PUD 1033f93067 PMD 0
[ 4555.979287] Oops: 0010 [#1] SMP
[ 4555.982809] Modules linked in: rpcrdma ib_isert iscsi_target_mod
target_core_mod ib_iser libiscsi scsi_transport_iscsi ib_ipoib rdma_ucm ib_ucm
ib_uverbs ib_umad rdma_cm ib_cm iw_cm dm_mirror dm_region_hash dm_log dm_mod
dax sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass
crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_simd
glue_helper cryptd hfi1 rdmavt iTCO_wdt iTCO_vendor_support ib_core mei_me
lpc_ich pcspkr mei ioatdma sg shpchp i2c_i801 mfd_core wmi ipmi_si ipmi_devintf
ipmi_msghandler acpi_power_meter acpi_pad nfsd auth_rpcgss nfs_acl lockd grace
sunrpc ip_tables ext4 mbcache jbd2 sd_mod mgag200 drm_kms_helper syscopyarea
sysfillrect sysimgblt fb_sys_fops ttm igb ahci crc32c_intel ptp libahci
pps_core drm dca libata i2c_algo_bit i2c_core
[ 4556.061190] CPU: 54 PID: 9841 Comm: ibacm Tainted: G          I
4.14.0-rc2+ #6
[ 4556.069667] Hardware name: Intel Corporation S2600WT2/S2600WT2, BIOS
SE5C610.86B.01.01.0008.021120151325 02/11/2015
[ 4556.081339] task: ffff880855f42d00 task.stack: ffffc900246b4000
[ 4556.087967] RIP: 0010:          (null)
[ 4556.092166] RSP: 0018:ffffc900246b7bc8 EFLAGS: 00010246
[ 4556.098018] RAX: ffffffff81dbe9e0 RBX: ffff881058bb1000 RCX:
0000000000000000
[ 4556.105997] RDX: 0000000000001100 RSI: ffff881058bb1320 RDI:
ffff881056362000
[ 4556.113984] RBP: ffffc900246b7bf8 R08: 0000000000000ec0 R09:
0000000000001100
[ 4556.121971] R10: ffff8810573a5000 R11: 0000000000000000 R12:
ffff881056362000
[ 4556.129957] R13: 0000000000000ec0 R14: ffff881058bb1320 R15:
0000000000000ec0
[ 4556.137945] FS:  00007fe0ba5a38c0(0000) GS:ffff88105f080000(0000)
knlGS:0000000000000000
[ 4556.147000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4556.153433] CR2: 0000000000000000 CR3: 0000001056f5d003 CR4:
00000000001606e0
[ 4556.161419] Call Trace:
[ 4556.164167]  ? netlink_dump+0x12c/0x290
[ 4556.168468]  __netlink_dump_start+0x186/0x1f0
[ 4556.173357]  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
[ 4556.178724]  rdma_nl_rcv+0xdc/0x130 [ib_core]
[ 4556.183604]  netlink_unicast+0x181/0x240
[ 4556.187998]  netlink_sendmsg+0x2c2/0x3b0
[ 4556.192392]  sock_sendmsg+0x38/0x50
[ 4556.196299]  SYSC_sendto+0x102/0x190
[ 4556.200308]  ? __audit_syscall_entry+0xaf/0x100
[ 4556.205387]  ? syscall_trace_enter+0x1d0/0x2b0
[ 4556.210366]  ? __audit_syscall_exit+0x209/0x290
[ 4556.215442]  SyS_sendto+0xe/0x10
[ 4556.219060]  do_syscall_64+0x67/0x1b0
[ 4556.223165]  entry_SYSCALL64_slow_path+0x25/0x25
[ 4556.228328] RIP: 0033:0x7fe0b9db2a63
[ 4556.232333] RSP: 002b:00007ffc55edc260 EFLAGS: 00000293 ORIG_RAX:
000000000000002c
[ 4556.240808] RAX: ffffffffffffffda RBX: 0000000000000010 RCX:
00007fe0b9db2a63
[ 4556.248796] RDX: 0000000000000010 RSI: 00007ffc55edc280 RDI:
000000000000000d
[ 4556.256782] RBP: 00007ffc55edc670 R08: 00007ffc55edc270 R09:
000000000000000c
[ 4556.265321] R10: 0000000000000000 R11: 0000000000000293 R12:
00007ffc55edc280
[ 4556.273846] R13: 000000000260b400 R14: 000000000000000d R15:
0000000000000001
[ 4556.282368] Code:  Bad RIP value.
[ 4556.286629] RIP:           (null) RSP: ffffc900246b7bc8
[ 4556.293013] CR2: 0000000000000000
[ 4556.297292] ---[ end trace 8d67abcfd10ec209 ]---
[ 4556.305465] Kernel panic - not syncing: Fatal exception
[ 4556.313786] Kernel Offset: disabled
[ 4556.321563] ---[ end Kernel panic - not syncing: Fatal exception
[ 4556.328960] ------------[ cut here ]------------

Only check for NLM_F_DUMP bits if the index != RDMA_NL_LS.

Additionally, make sure that the .dump() callback is not NULL
before calling it.

INTERNAL: PR 140882 - Machine unusable after doing ping

Change-Id: Ia589658d109d72d927857ecb1ab79babe84da895
Fixes: 647c75ac59a48a54 ("RDMA/netlink: Convert LS to doit callback")
Reviewed-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/core/netlink.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
index b12e587..7133947 100644
--- a/drivers/infiniband/core/netlink.c
+++ b/drivers/infiniband/core/netlink.c
@@ -176,11 +176,14 @@ static int rdma_nl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EPERM;
 
 	/* FIXME: Convert IWCM to properly handle doit callbacks */
-	if ((nlh->nlmsg_flags & NLM_F_DUMP) || index == RDMA_NL_RDMA_CM ||
-	    index == RDMA_NL_IWCM) {
+	if ((index != RDMA_NL_LS) &&
+	    ((nlh->nlmsg_flags & NLM_F_DUMP) || index == RDMA_NL_RDMA_CM ||
+	     index == RDMA_NL_IWCM)) {
 		struct netlink_dump_control c = {
 			.dump = cb_table[op].dump,
 		};
+		if (!c.dump)
+			return 0;
 		return netlink_dump_start(nls, skb, nlh, &c);
 	}
 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 5644 bytes --]

On Thu, Oct 19, 2017 at 05:40:59PM -0400, Michael J. Ruhl wrote:
> I was playing with the ibacm service and discovered an issue
> the other day.
>
> If no provider library is present (I removed libacmp.so, and the
> provider keyword in the opts.cfg file is libacmp), when a resolve
> request is posted, the kernel will crash with the following Oops:
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP:           (null)
> PGD 10543f1067 P4D 10543f1067 PUD 1033f93067 PMD 0
> Oops: 0010 [#1] SMP
> Modules linked in: rpcrdma ib_isert iscsi_target_mod
>  target_core_mod ib_iser libiscsi scsi_transport_iscsi ib_ipoib rdma_ucm ib_u
>  ib_uverbs ib_umad rdma_cm ib_cm iw_cm dm_mirror dm_region_hash dm_log dm_mod
>  dax sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass
>  crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_si
>  glue_helper cryptd hfi1 rdmavt iTCO_wdt iTCO_vendor_support ib_core mei_me
>  lpc_ich pcspkr mei ioatdma sg shpchp i2c_i801 mfd_core wmi ipmi_si ipmi_devi
>  ipmi_msghandler acpi_power_meter acpi_pad nfsd auth_rpcgss nfs_acl lockd gra
>  sunrpc ip_tables ext4 mbcache jbd2 sd_mod mgag200 drm_kms_helper syscopyarea
>  sysfillrect sysimgblt fb_sys_fops ttm igb ahci crc32c_intel ptp libahci
>  pps_core drm dca libata i2c_algo_bit i2c_core
> CPU: 54 PID: 9841 Comm: ibacm Tainted: G          I    4.14.0-rc2+ #6
> Hardware name: Intel Corporation S2600WT2/S2600WT2, BIOS     SE5C610.86B.01.01.0008.021120151325 02/11/2015
> task: ffff880855f42d00 task.stack: ffffc900246b4000
> RIP: 0010:          (null)
> RSP: 0018:ffffc900246b7bc8 EFLAGS: 00010246
> RAX: ffffffff81dbe9e0 RBX: ffff881058bb1000 RCX: 0000000000000000
> RDX: 0000000000001100 RSI: ffff881058bb1320 RDI: ffff881056362000
> RBP: ffffc900246b7bf8 R08: 0000000000000ec0 R09: 0000000000001100
> R10: ffff8810573a5000 R11: 0000000000000000 R12: ffff881056362000
> R13: 0000000000000ec0 R14: ffff881058bb1320 R15: 0000000000000ec0
> FS:  00007fe0ba5a38c0(0000) GS:ffff88105f080000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000001056f5d003 CR4: 00000000001606e0
> Call Trace:
>  ? netlink_dump+0x12c/0x290
>  __netlink_dump_start+0x186/0x1f0
>  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
>  rdma_nl_rcv+0xdc/0x130 [ib_core]
>  netlink_unicast+0x181/0x240
>  netlink_sendmsg+0x2c2/0x3b0
>  sock_sendmsg+0x38/0x50
>  SYSC_sendto+0x102/0x190
>  ? __audit_syscall_entry+0xaf/0x100
>  ? syscall_trace_enter+0x1d0/0x2b0
>  ? __audit_syscall_exit+0x209/0x290
>  SyS_sendto+0xe/0x10
>  do_syscall_64+0x67/0x1b0
>  entry_SYSCALL64_slow_path+0x25/0x25
> RIP: 0033:0x7fe0b9db2a63
> RSP: 002b:00007ffc55edc260 EFLAGS: 00000293 ORIG_RAX: 000000000000002c
> RAX: ffffffffffffffda RBX: 0000000000000010 RCX: 00007fe0b9db2a63
> RDX: 0000000000000010 RSI: 00007ffc55edc280 RDI: 000000000000000d
> RBP: 00007ffc55edc670 R08: 00007ffc55edc270 R09: 000000000000000c
> R10: 0000000000000000 R11: 0000000000000293 R12: 00007ffc55edc280
> R13: 000000000260b400 R14: 000000000000000d R15: 0000000000000001
> Code:  Bad RIP value.
> RIP:           (null) RSP: ffffc900246b7bc8
> CR2: 0000000000000000
> ---[ end trace 8d67abcfd10ec209 ]---
> Kernel panic - not syncing: Fatal exception
> Kernel Offset: disabled
> ---[ end Kernel panic - not syncing: Fatal exception
> ------------[ cut here ]------------
>
> The issue is that in rdma_nl_rcv_msg(), the check
> 'if (flags & NLM_F_DUMP)' is not completely correct.
>
> NLM_F_DUMP is two bits NLM_F_ROOT | NLM_F_MATCH.
>
> ibacm sends a RDMA_NL_LS response with the RDMA_NL_LS_F_ERR bit set
> if an error occurs in the service (like no provider being available,
> or ACM_STATUS_ENODATA, etc.).
>
> NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
>
> The current code thinks that it sees a NLM_F_DUMP flag and incorrectly calls
> the .dump() callback.

Hi Michael,

Thanks for the report and for excellent analysis, You are right that
RDMA_NL_LS_F_ERR has the same value as NLM_F_ROOT and it is bad, but
I just think that it is not the final root cause.

In case of errors, the LS was supposed to send NLMSG_ERROR message and not
overload general nlmsg_flags, which is awful. However I don't know if it is
feasible to fix current implementation without breaking UAPI contract.

In meanwhile, can we implement dummy dumpit functions for the LS,
which reuse ib_nl_is_good_ip_resp?

I prefer this solution over yours, because it doesn't mix LS-specifics with
general decision function and leaves LS anomalies in the LS-relevant code.

And returning 0 in absence of dumpit function as a response with
NLM_F_DUMP flag is wrong. User should be aware of the fact that
something wrong was with his request.

Thanks

>
> The included patch is an atempt to fix this issue.  This patch fixes the
> issue that I am seeing, but I am not sure how to test the messages for
> RDMA_NL_RDMA_CM or RDMA_NL_IWCM (or any message that uses the
> NLM_F_DUMP bits).
>
> If anyone has some knowledge of these services, any extra testing would
> be welcomed.
>
> If the patch has no issues or comments, I will formally re-submit it
> (through my usual channel Denny).
>
> Thanks,
>
> Mike
>
>
> ---
>
> Michael J. Ruhl (1):
>       RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag
>
>
>  drivers/infiniband/core/netlink.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
>
> --
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Wan, Kaike]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
> Sent: Friday, October 20, 2017 3:37 AM
> To: Ruhl, Michael J
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> misinterpreted flag
> >
> > The issue is that in rdma_nl_rcv_msg(), the check 'if (flags &
> > NLM_F_DUMP)' is not completely correct.
> >
> > NLM_F_DUMP is two bits NLM_F_ROOT | NLM_F_MATCH.
> >
> > ibacm sends a RDMA_NL_LS response with the RDMA_NL_LS_F_ERR bit set
> if
> > an error occurs in the service (like no provider being available, or
> > ACM_STATUS_ENODATA, etc.).
> >
> > NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
> >
> > The current code thinks that it sees a NLM_F_DUMP flag and incorrectly
> > calls the .dump() callback.
> 
> Hi Michael,
> 
> Thanks for the report and for excellent analysis, You are right that
> RDMA_NL_LS_F_ERR has the same value as NLM_F_ROOT and it is bad, but I
> just think that it is not the final root cause.
> 
> In case of errors, the LS was supposed to send NLMSG_ERROR message and
> not overload general nlmsg_flags, which is awful. However I don't know if it
> is feasible to fix current implementation without breaking UAPI contract.

The nlmsg_flags from 0x100 and up have always been overloaded for different requests, as shown in include/uapi/linux/netlink.h:

* Modifiers to GET request */
#define NLM_F_ROOT	0x100	/* specify tree	root	*/
#define NLM_F_MATCH	0x200	/* return all matching	*/
#define NLM_F_ATOMIC	0x400	/* atomic GET		*/
#define NLM_F_DUMP	(NLM_F_ROOT|NLM_F_MATCH)

/* Modifiers to NEW request */
#define NLM_F_REPLACE	0x100	/* Override existing		*/
#define NLM_F_EXCL	0x200	/* Do not touch, if it exists	*/
#define NLM_F_CREATE	0x400	/* Create, if it does not exist	*/
#define NLM_F_APPEND	0x800	/* Add to end of list		*/

/* Modifiers to DELETE request */
#define NLM_F_NONREC	0x100	/* Do not delete recursively	*/

/* Flags for ACK message */
#define NLM_F_CAPPED	0x100	/* request was capped */
#define NLM_F_ACK_TLVS	0x200	/* extended ACK TVLs were included *

The NLM_F_DUMP flag is supposed to be used for the GET request only, not as a general flag for all netlink requests.

> 
> In meanwhile, can we implement dummy dumpit functions for the LS, which
> reuse ib_nl_is_good_ip_resp?

Why do you want to jump all the dump hoops instead of directly calling the response handler? LS is different from other netlink channels in that it sends request from kernel to user space and receives responses from it instead of the other way around. Consequently, the handling of netlink responses may be different from handing requests from user space.

> 
> I prefer this solution over yours, because it doesn't mix LS-specifics with
> general decision function and leaves LS anomalies in the LS-relevant code.
> 
> And returning 0 in absence of dumpit function as a response with
> NLM_F_DUMP flag is wrong. User should be aware of the fact that
> something wrong was with his request.
> 
> Thanks
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3744 bytes --]

On Fri, Oct 20, 2017 at 12:18:02PM +0000, Wan, Kaike wrote:
>
>
> > -----Original Message-----
> > From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> > owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
> > Sent: Friday, October 20, 2017 3:37 AM
> > To: Ruhl, Michael J
> > Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> > misinterpreted flag
> > >
> > > The issue is that in rdma_nl_rcv_msg(), the check 'if (flags &
> > > NLM_F_DUMP)' is not completely correct.
> > >
> > > NLM_F_DUMP is two bits NLM_F_ROOT | NLM_F_MATCH.
> > >
> > > ibacm sends a RDMA_NL_LS response with the RDMA_NL_LS_F_ERR bit set
> > if
> > > an error occurs in the service (like no provider being available, or
> > > ACM_STATUS_ENODATA, etc.).
> > >
> > > NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
> > >
> > > The current code thinks that it sees a NLM_F_DUMP flag and incorrectly
> > > calls the .dump() callback.
> >
> > Hi Michael,
> >
> > Thanks for the report and for excellent analysis, You are right that
> > RDMA_NL_LS_F_ERR has the same value as NLM_F_ROOT and it is bad, but I
> > just think that it is not the final root cause.
> >
> > In case of errors, the LS was supposed to send NLMSG_ERROR message and
> > not overload general nlmsg_flags, which is awful. However I don't know if it
> > is feasible to fix current implementation without breaking UAPI contract.
>
> The nlmsg_flags from 0x100 and up have always been overloaded for different requests, as shown in include/uapi/linux/netlink.h:

Exactly, they are overloaded in include/uapi/linux/netlink.h where all
nlmsg flags are declared and not in some random rdma*.h file.

But it is not my main point.

My main point is lack of usage of NLMSG_ERROR messages to inform about
errors, exactly as kernel informs users about errors in netlink, but in the
opposite direction.

>
> * Modifiers to GET request */
> #define NLM_F_ROOT	0x100	/* specify tree	root	*/
> #define NLM_F_MATCH	0x200	/* return all matching	*/
> #define NLM_F_ATOMIC	0x400	/* atomic GET		*/
> #define NLM_F_DUMP	(NLM_F_ROOT|NLM_F_MATCH)
>
> /* Modifiers to NEW request */
> #define NLM_F_REPLACE	0x100	/* Override existing		*/
> #define NLM_F_EXCL	0x200	/* Do not touch, if it exists	*/
> #define NLM_F_CREATE	0x400	/* Create, if it does not exist	*/
> #define NLM_F_APPEND	0x800	/* Add to end of list		*/
>
> /* Modifiers to DELETE request */
> #define NLM_F_NONREC	0x100	/* Do not delete recursively	*/
>
> /* Flags for ACK message */
> #define NLM_F_CAPPED	0x100	/* request was capped */
> #define NLM_F_ACK_TLVS	0x200	/* extended ACK TVLs were included *
>
> The NLM_F_DUMP flag is supposed to be used for the GET request only, not as a general flag for all netlink requests.
>
> >
> > In meanwhile, can we implement dummy dumpit functions for the LS, which
> > reuse ib_nl_is_good_ip_resp?
>
> Why do you want to jump all the dump hoops instead of directly calling the response handler? LS is different from other netlink channels in that it sends request from kernel to user space and receives responses from it instead of the other way around. Consequently, the handling of netlink responses may be different from handing requests from user space.
>

Doesn't the part of email below answers on the question "why"?

> >
> > I prefer this solution over yours, because it doesn't mix LS-specifics with
> > general decision function and leaves LS anomalies in the LS-relevant code.


> >
> > And returning 0 in absence of dumpit function as a response with
> > NLM_F_DUMP flag is wrong. User should be aware of the fact that
> > something wrong was with his request.
> >
> > Thanks
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Ruhl, Michael J]

> -----Original Message-----
> From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Friday, October 20, 2017 3:37 AM
> To: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> misinterpreted flag
> 
> On Thu, Oct 19, 2017 at 05:40:59PM -0400, Michael J. Ruhl wrote:
> > I was playing with the ibacm service and discovered an issue
> > the other day.
> >
> > If no provider library is present (I removed libacmp.so, and the
> > provider keyword in the opts.cfg file is libacmp), when a resolve
> > request is posted, the kernel will crash with the following Oops:
> >
> > Call Trace:
> >  ? netlink_dump+0x12c/0x290
> >  __netlink_dump_start+0x186/0x1f0
> >  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
> >  rdma_nl_rcv+0xdc/0x130 [ib_core]
> >  netlink_unicast+0x181/0x240
> >  netlink_sendmsg+0x2c2/0x3b0
> >  sock_sendmsg+0x38/0x50
> >  SYSC_sendto+0x102/0x190
> >  ? __audit_syscall_entry+0xaf/0x100
> >  ? syscall_trace_enter+0x1d0/0x2b0
> >  ? __audit_syscall_exit+0x209/0x290
> >  SyS_sendto+0xe/0x10
> >  do_syscall_64+0x67/0x1b0
> >  entry_SYSCALL64_slow_path+0x25/0x25
> >
> > The issue is that in rdma_nl_rcv_msg(), the check
> > 'if (flags & NLM_F_DUMP)' is not completely correct.
> >
> > NLM_F_DUMP is two bits NLM_F_ROOT | NLM_F_MATCH.
> >
> > ibacm sends a RDMA_NL_LS response with the RDMA_NL_LS_F_ERR bit set
> > if an error occurs in the service (like no provider being available,
> > or ACM_STATUS_ENODATA, etc.).
> >
> > NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
> >
> > The current code thinks that it sees a NLM_F_DUMP flag and incorrectly calls
> > the .dump() callback.
> 
> Hi Michael,
> 
> Thanks for the report and for excellent analysis, You are right that
> RDMA_NL_LS_F_ERR has the same value as NLM_F_ROOT and it is bad, but
> I just think that it is not the final root cause.
> 
> In case of errors, the LS was supposed to send NLMSG_ERROR message and not
> overload general nlmsg_flags, which is awful. However I don't know if it is
> feasible to fix current implementation without breaking UAPI contract.

I agree this is probably something that needs to get followed up on.
 
> In meanwhile, can we implement dummy dumpit functions for the LS,
> which reuse ib_nl_is_good_ip_resp?

The original code does not call the netlink_dump_start() code for this path, so if we create a dummy dump function we will have to add code to special case this and call it directly.

So maybe we could just go back to the original code and call .doit rather than .dump in the non netlink_dump_start() path? 

i.e.:
      if (!(nlh->nlmsg_flags & NLM_F_REQUEST) ||
           (index == RDMA_NL_LS && op == RDMA_NL_LS_OP_SET_TIMEOUT)) {
               cb.skb = skb;
               cb.nlh = nlh;
               cb.dump = cb_table[op].dump;
-               return cb.dump(skb, &cb);
+               return cb.doit(skb, &cb);
       } else {
               c.dump = cb_table[op].dump;


> I prefer this solution over yours, because it doesn't mix LS-specifics with
> general decision function and leaves LS anomalies in the LS-relevant code.

The problem with this is that the code has to take into consideration the LS anomalies either before the this function is called, or has to deal with them here.

The other thought would be to special case the LS stuff before this decision and call the .doit function there (and then return).
 
> And returning 0 in absence of dumpit function as a response with
> NLM_F_DUMP flag is wrong. User should be aware of the fact that
> something wrong was with his request.

This was the value that the function returns if .dump and .doit are NOT selected, so I thought that this was appropriate.  Should that value (the final return 0) be changed to something different?

Mike
 
> Thanks
> 
> >
> > The included patch is an atempt to fix this issue.  This patch fixes the
> > issue that I am seeing, but I am not sure how to test the messages for
> > RDMA_NL_RDMA_CM or RDMA_NL_IWCM (or any message that uses the
> > NLM_F_DUMP bits).
> >
> > If anyone has some knowledge of these services, any extra testing would
> > be welcomed.
> >
> > If the patch has no issues or comments, I will formally re-submit it
> > (through my usual channel Denny).
> >
> > Thanks,
> >
> > Mike
> >
> >
> > ---
> >
> > Michael J. Ruhl (1):
> >       RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag
> >
> >
> >  drivers/infiniband/core/netlink.c |    7 +++++--
> >  1 files changed, 5 insertions(+), 2 deletions(-)
> >
> > --
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Wan, Kaike]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
> Sent: Friday, October 20, 2017 12:20 PM
> To: Wan, Kaike
> Cc: Ruhl, Michael J; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> misinterpreted flag
> 
> On Fri, Oct 20, 2017 at 12:18:02PM +0000, Wan, Kaike wrote:
> > The nlmsg_flags from 0x100 and up have always been overloaded for
> different requests, as shown in include/uapi/linux/netlink.h:
> 
> Exactly, they are overloaded in include/uapi/linux/netlink.h where all nlmsg
> flags are declared and not in some random rdma*.h file.

Since they are known to be overloaded, how could you depend on the flags to make decision for all requests/responses?

The reason for defining RDMA_NL_LS_F_ERR in include/uapi/rdma/rdma_netlink is that all LS related defines and structures are defined in the file. It is true that it can lead to misuse, just like the current code. However, the defines in include/uapi/linux/netlink.h un-mistakenly indicate that those bits can be overloaded. 

> 
> But it is not my main point.
> 
> My main point is lack of usage of NLMSG_ERROR messages to inform about
> errors, exactly as kernel informs users about errors in netlink, but in the
> opposite direction.

The difference is that the LS needs all the original information (request type, client index, msg_seq, etc) to match the response with the original request. Therefore, the easiest thing to do is to return the response with the original info plus an error flag, instead of using the NLMSG_ERROR message.

> > >
> > > In meanwhile, can we implement dummy dumpit functions for the LS,
> > > which reuse ib_nl_is_good_ip_resp?
> >
> > Why do you want to jump all the dump hoops instead of directly calling the
> response handler? LS is different from other netlink channels in that it sends
> request from kernel to user space and receives responses from it instead of
> the other way around. Consequently, the handling of netlink responses may
> be different from handing requests from user space.
> >
> 
> Doesn't the part of email below answers on the question "why"?

No. A response should not necessarily be processed the same as a request.

> 
> > >
> > > I prefer this solution over yours, because it doesn't mix
> > > LS-specifics with general decision function and leaves LS anomalies in the
> LS-relevant code.
> 
> 
> > >
> > > And returning 0 in absence of dumpit function as a response with
> > > NLM_F_DUMP flag is wrong. User should be aware of the fact that
> > > something wrong was with his request.
> > >
> > > Thanks
> > >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3527 bytes --]

On Fri, Oct 20, 2017 at 07:04:33PM +0000, Wan, Kaike wrote:
>
>
> > -----Original Message-----
> > From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> > owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
> > Sent: Friday, October 20, 2017 12:20 PM
> > To: Wan, Kaike
> > Cc: Ruhl, Michael J; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> > misinterpreted flag
> >
> > On Fri, Oct 20, 2017 at 12:18:02PM +0000, Wan, Kaike wrote:
> > > The nlmsg_flags from 0x100 and up have always been overloaded for
> > different requests, as shown in include/uapi/linux/netlink.h:
> >
> > Exactly, they are overloaded in include/uapi/linux/netlink.h where all nlmsg
> > flags are declared and not in some random rdma*.h file.
>
> Since they are known to be overloaded, how could you depend on the flags to make decision for all requests/responses?

Sorry for applying common sense for the RDMA subsystem. The netlink structures are declared
and exported to users in one specific file, the structure, flags and defines belong to netdev
subsystem and were not supposed to be overwritten by other users.

>
> The reason for defining RDMA_NL_LS_F_ERR in include/uapi/rdma/rdma_netlink is that all LS related defines and structures are defined in the file. It is true that it can lead to misuse, just like the current code. However, the defines in include/uapi/linux/netlink.h un-mistakenly indicate that those bits can be overloaded.

We are talking about theoretical case, the current LS implementation
will stay as is, so no worries.

>
> >
> > But it is not my main point.
> >
> > My main point is lack of usage of NLMSG_ERROR messages to inform about
> > errors, exactly as kernel informs users about errors in netlink, but in the
> > opposite direction.
>
> The difference is that the LS needs all the original information (request type, client index, msg_seq, etc) to match the response with the original request. Therefore, the easiest thing to do is to return the response with the original info plus an error flag, instead of using the NLMSG_ERROR message.
>

Just wanted to mention that NLMSG_ERROR is netlink message type and it
carries all other information except message type.

> > > >
> > > > In meanwhile, can we implement dummy dumpit functions for the LS,
> > > > which reuse ib_nl_is_good_ip_resp?
> > >
> > > Why do you want to jump all the dump hoops instead of directly calling the
> > response handler? LS is different from other netlink channels in that it sends
> > request from kernel to user space and receives responses from it instead of
> > the other way around. Consequently, the handling of netlink responses may
> > be different from handing requests from user space.
> > >
> >
> > Doesn't the part of email below answers on the question "why"?
>
> No. A response should not necessarily be processed the same as a request.

Right, and I expected to see complete mirror of kernel-to-user
communication in respect to LS user-to-kernel communication.

>
> >
> > > >
> > > > I prefer this solution over yours, because it doesn't mix
> > > > LS-specifics with general decision function and leaves LS anomalies in the
> > LS-relevant code.
> >
> >
> > > >
> > > > And returning 0 in absence of dumpit function as a response with
> > > > NLM_F_DUMP flag is wrong. User should be aware of the fact that
> > > > something wrong was with his request.
> > > >
> > > > Thanks
> > > >
> > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 6126 bytes --]

On Fri, Oct 20, 2017 at 05:20:22PM +0000, Ruhl, Michael J wrote:
> > -----Original Message-----
> > From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> > Sent: Friday, October 20, 2017 3:37 AM
> > To: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> > misinterpreted flag
> >
> > On Thu, Oct 19, 2017 at 05:40:59PM -0400, Michael J. Ruhl wrote:
> > > I was playing with the ibacm service and discovered an issue
> > > the other day.
> > >
> > > If no provider library is present (I removed libacmp.so, and the
> > > provider keyword in the opts.cfg file is libacmp), when a resolve
> > > request is posted, the kernel will crash with the following Oops:
> > >
> > > Call Trace:
> > >  ? netlink_dump+0x12c/0x290
> > >  __netlink_dump_start+0x186/0x1f0
> > >  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
> > >  rdma_nl_rcv+0xdc/0x130 [ib_core]
> > >  netlink_unicast+0x181/0x240
> > >  netlink_sendmsg+0x2c2/0x3b0
> > >  sock_sendmsg+0x38/0x50
> > >  SYSC_sendto+0x102/0x190
> > >  ? __audit_syscall_entry+0xaf/0x100
> > >  ? syscall_trace_enter+0x1d0/0x2b0
> > >  ? __audit_syscall_exit+0x209/0x290
> > >  SyS_sendto+0xe/0x10
> > >  do_syscall_64+0x67/0x1b0
> > >  entry_SYSCALL64_slow_path+0x25/0x25
> > >
> > > The issue is that in rdma_nl_rcv_msg(), the check
> > > 'if (flags & NLM_F_DUMP)' is not completely correct.
> > >
> > > NLM_F_DUMP is two bits NLM_F_ROOT | NLM_F_MATCH.
> > >
> > > ibacm sends a RDMA_NL_LS response with the RDMA_NL_LS_F_ERR bit set
> > > if an error occurs in the service (like no provider being available,
> > > or ACM_STATUS_ENODATA, etc.).
> > >
> > > NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
> > >
> > > The current code thinks that it sees a NLM_F_DUMP flag and incorrectly calls
> > > the .dump() callback.
> >
> > Hi Michael,
> >
> > Thanks for the report and for excellent analysis, You are right that
> > RDMA_NL_LS_F_ERR has the same value as NLM_F_ROOT and it is bad, but
> > I just think that it is not the final root cause.
> >
> > In case of errors, the LS was supposed to send NLMSG_ERROR message and not
> > overload general nlmsg_flags, which is awful. However I don't know if it is
> > feasible to fix current implementation without breaking UAPI contract.
>
> I agree this is probably something that needs to get followed up on.
>
> > In meanwhile, can we implement dummy dumpit functions for the LS,
> > which reuse ib_nl_is_good_ip_resp?
>
> The original code does not call the netlink_dump_start() code for this path, so if we create a dummy dump function we will have to add code to special case this and call it directly.
>
> So maybe we could just go back to the original code and call .doit rather than .dump in the non netlink_dump_start() path?
>
> i.e.:
>       if (!(nlh->nlmsg_flags & NLM_F_REQUEST) ||
>            (index == RDMA_NL_LS && op == RDMA_NL_LS_OP_SET_TIMEOUT)) {
>                cb.skb = skb;
>                cb.nlh = nlh;
>                cb.dump = cb_table[op].dump;
> -               return cb.dump(skb, &cb);
> +               return cb.doit(skb, &cb);
>        } else {
>                c.dump = cb_table[op].dump;
>
>
> > I prefer this solution over yours, because it doesn't mix LS-specifics with
> > general decision function and leaves LS anomalies in the LS-relevant code.
>
> The problem with this is that the code has to take into consideration the LS anomalies either before the this function is called, or has to deal with them here.
>
> The other thought would be to special case the LS stuff before this decision and call the .doit function there (and then return).

What about such code?

diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
index b12e58787c3d..6a7362664876 100644
--- a/drivers/infiniband/core/netlink.c
+++ b/drivers/infiniband/core/netlink.c
@@ -175,6 +175,14 @@ static int rdma_nl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	    !netlink_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;

+	/*
+	 * LS is special because it runs backward communication
+	 * and it overloads NLM_F_DUMP flag with RDMA_NL_LS_F_ERR
+	 * So we are calling to .doit before processing .dumpit call.
+	 */
+	if (index == RDMA_NL_LS)
+		return cb_table[op].doit(skb, nlh, extack);
+
 	/* FIXME: Convert IWCM to properly handle doit callbacks */
 	if ((nlh->nlmsg_flags & NLM_F_DUMP) || index == RDMA_NL_RDMA_CM ||
 	    index == RDMA_NL_IWCM) {

>
> > And returning 0 in absence of dumpit function as a response with
> > NLM_F_DUMP flag is wrong. User should be aware of the fact that
> > something wrong was with his request.
>
> This was the value that the function returns if .dump and .doit are NOT selected, so I thought that this was appropriate.  Should that value (the final return 0) be changed to something different?

The is_nl_valid() should return "false" If no .dumpit/.doit functions exist.
In such case, we are supposed to return EINVAL.

Thanks

>
> Mike
>
> > Thanks
> >
> > >
> > > The included patch is an atempt to fix this issue.  This patch fixes the
> > > issue that I am seeing, but I am not sure how to test the messages for
> > > RDMA_NL_RDMA_CM or RDMA_NL_IWCM (or any message that uses the
> > > NLM_F_DUMP bits).
> > >
> > > If anyone has some knowledge of these services, any extra testing would
> > > be welcomed.
> > >
> > > If the patch has no issues or comments, I will formally re-submit it
> > > (through my usual channel Denny).
> > >
> > > Thanks,
> > >
> > > Mike
> > >
> > >
> > > ---
> > >
> > > Michael J. Ruhl (1):
> > >       RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag
> > >
> > >
> > >  drivers/infiniband/core/netlink.c |    7 +++++--
> > >  1 files changed, 5 insertions(+), 2 deletions(-)
> > >
> > > --
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Ruhl, Michael J]

> -----Original Message-----
> From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Monday, October 23, 2017 4:11 AM
> To: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> misinterpreted flag
> 
> On Fri, Oct 20, 2017 at 05:20:22PM +0000, Ruhl, Michael J wrote:
> > > On Thu, Oct 19, 2017 at 05:40:59PM -0400, Michael J. Ruhl wrote:
> > > > I was playing with the ibacm service and discovered an issue
> > > > the other day.
> > > >
> > > > If no provider library is present (I removed libacmp.so, and the
> > > > provider keyword in the opts.cfg file is libacmp), when a resolve
> > > > request is posted, the kernel will crash with the following Oops:
> > > >
> > > > Call Trace:
> > > >  ? netlink_dump+0x12c/0x290
> > > >  __netlink_dump_start+0x186/0x1f0
> > > >  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
> > > >  rdma_nl_rcv+0xdc/0x130 [ib_core]
> > > >  netlink_unicast+0x181/0x240
> > > >  netlink_sendmsg+0x2c2/0x3b0
> > > >  sock_sendmsg+0x38/0x50
> > > >  SYSC_sendto+0x102/0x190
> > > >  ? __audit_syscall_entry+0xaf/0x100
> > > >  ? syscall_trace_enter+0x1d0/0x2b0
> > > >  ? __audit_syscall_exit+0x209/0x290
> > > >  SyS_sendto+0xe/0x10
> > > >  do_syscall_64+0x67/0x1b0
> > > >  entry_SYSCALL64_slow_path+0x25/0x25
> > > >
> > > > The issue is that in rdma_nl_rcv_msg(), the check
> > > > 'if (flags & NLM_F_DUMP)' is not completely correct.
> > > >
> > > > NLM_F_DUMP is two bits NLM_F_ROOT | NLM_F_MATCH.
> > > >
> > > > ibacm sends a RDMA_NL_LS response with the RDMA_NL_LS_F_ERR bit set
> > > > if an error occurs in the service (like no provider being available,
> > > > or ACM_STATUS_ENODATA, etc.).
> > > >
> > > > NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
> > > >
> > > > The current code thinks that it sees a NLM_F_DUMP flag and incorrectly
> calls
> > > > the .dump() callback.
> > >
> > > Hi Michael,
> > >
> > > Thanks for the report and for excellent analysis, You are right that
> > > RDMA_NL_LS_F_ERR has the same value as NLM_F_ROOT and it is bad, but
> > > I just think that it is not the final root cause.
> > >
> > > In case of errors, the LS was supposed to send NLMSG_ERROR message and
> not
> > > overload general nlmsg_flags, which is awful. However I don't know if it is
> > > feasible to fix current implementation without breaking UAPI contract.
> >
> > I agree this is probably something that needs to get followed up on.
> >
> > > In meanwhile, can we implement dummy dumpit functions for the LS,
> > > which reuse ib_nl_is_good_ip_resp?
> >
> > The original code does not call the netlink_dump_start() code for this path, so
> if we create a dummy dump function we will have to add code to special case
> this and call it directly.
> >
> > So maybe we could just go back to the original code and call .doit rather than
> .dump in the non netlink_dump_start() path?
> >
> > i.e.:
> >       if (!(nlh->nlmsg_flags & NLM_F_REQUEST) ||
> >            (index == RDMA_NL_LS && op == RDMA_NL_LS_OP_SET_TIMEOUT)) {
> >                cb.skb = skb;
> >                cb.nlh = nlh;
> >                cb.dump = cb_table[op].dump;
> > -               return cb.dump(skb, &cb);
> > +               return cb.doit(skb, &cb);
> >        } else {
> >                c.dump = cb_table[op].dump;
> >
> >
> > > I prefer this solution over yours, because it doesn't mix LS-specifics with
> > > general decision function and leaves LS anomalies in the LS-relevant code.
> >
> > The problem with this is that the code has to take into consideration the LS
> anomalies either before the this function is called, or has to deal with them
> here.
> >
> > The other thought would be to special case the LS stuff before this decision
> and call the .doit function there (and then return).
> 
> What about such code?
> 
> diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
> index b12e58787c3d..6a7362664876 100644
> --- a/drivers/infiniband/core/netlink.c
> +++ b/drivers/infiniband/core/netlink.c
> @@ -175,6 +175,14 @@ static int rdma_nl_rcv_msg(struct sk_buff *skb, struct
> nlmsghdr *nlh,
>  	    !netlink_capable(skb, CAP_NET_ADMIN))
>  		return -EPERM;
> 
> +	/*
> +	 * LS is special because it runs backward communication
> +	 * and it overloads NLM_F_DUMP flag with RDMA_NL_LS_F_ERR
> +	 * So we are calling to .doit before processing .dumpit call.
> +	 */
> +	if (index == RDMA_NL_LS)
> +		return cb_table[op].doit(skb, nlh, extack);
> +

Hi Leon,

That works for me (I am going to wordsmith the comment slightly).
A new patch will be forthcoming shortly.

Mike

>  	/* FIXME: Convert IWCM to properly handle doit callbacks */
>  	if ((nlh->nlmsg_flags & NLM_F_DUMP) || index ==
> RDMA_NL_RDMA_CM ||
>  	    index == RDMA_NL_IWCM) {
> 
> >
> > > And returning 0 in absence of dumpit function as a response with
> > > NLM_F_DUMP flag is wrong. User should be aware of the fact that
> > > something wrong was with his request.
> >
> > This was the value that the function returns if .dump and .doit are NOT
> selected, so I thought that this was appropriate.  Should that value (the final
> return 0) be changed to something different?
> 
> The is_nl_valid() should return "false" If no .dumpit/.doit functions exist.
> In such case, we are supposed to return EINVAL.
> Thanks
> 
> >
> > Mike
> >
> > > Thanks
> > >
> > > >
> > > > The included patch is an atempt to fix this issue.  This patch fixes the
> > > > issue that I am seeing, but I am not sure how to test the messages for
> > > > RDMA_NL_RDMA_CM or RDMA_NL_IWCM (or any message that uses the
> > > > NLM_F_DUMP bits).
> > > >
> > > > If anyone has some knowledge of these services, any extra testing would
> > > > be welcomed.
> > > >
> > > > If the patch has no issues or comments, I will formally re-submit it
> > > > (through my usual channel Denny).
> > > >
> > > > Thanks,
> > > >
> > > > Mike
> > > >
> > > >
> > > > ---
> > > >
> > > > Michael J. Ruhl (1):
> > > >       RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag
> > > >
> > > >
> > > >  drivers/infiniband/core/netlink.c |    7 +++++--
> > > >  1 files changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > --
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 6437 bytes --]

On 10/23/2017 4:11 AM, Leon Romanovsky wrote:
> On Fri, Oct 20, 2017 at 05:20:22PM +0000, Ruhl, Michael J wrote:
>>> -----Original Message-----
>>> From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
>>> Sent: Friday, October 20, 2017 3:37 AM
>>> To: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
>>> misinterpreted flag
>>>
>>> On Thu, Oct 19, 2017 at 05:40:59PM -0400, Michael J. Ruhl wrote:
>>>> I was playing with the ibacm service and discovered an issue
>>>> the other day.
>>>>
>>>> If no provider library is present (I removed libacmp.so, and the
>>>> provider keyword in the opts.cfg file is libacmp), when a resolve
>>>> request is posted, the kernel will crash with the following Oops:
>>>>
>>>> Call Trace:
>>>>  ? netlink_dump+0x12c/0x290
>>>>  __netlink_dump_start+0x186/0x1f0
>>>>  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
>>>>  rdma_nl_rcv+0xdc/0x130 [ib_core]
>>>>  netlink_unicast+0x181/0x240
>>>>  netlink_sendmsg+0x2c2/0x3b0
>>>>  sock_sendmsg+0x38/0x50
>>>>  SYSC_sendto+0x102/0x190
>>>>  ? __audit_syscall_entry+0xaf/0x100
>>>>  ? syscall_trace_enter+0x1d0/0x2b0
>>>>  ? __audit_syscall_exit+0x209/0x290
>>>>  SyS_sendto+0xe/0x10
>>>>  do_syscall_64+0x67/0x1b0
>>>>  entry_SYSCALL64_slow_path+0x25/0x25
>>>>
>>>> The issue is that in rdma_nl_rcv_msg(), the check
>>>> 'if (flags & NLM_F_DUMP)' is not completely correct.
>>>>
>>>> NLM_F_DUMP is two bits NLM_F_ROOT | NLM_F_MATCH.
>>>>
>>>> ibacm sends a RDMA_NL_LS response with the RDMA_NL_LS_F_ERR bit set
>>>> if an error occurs in the service (like no provider being available,
>>>> or ACM_STATUS_ENODATA, etc.).
>>>>
>>>> NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
>>>>
>>>> The current code thinks that it sees a NLM_F_DUMP flag and incorrectly calls
>>>> the .dump() callback.
>>>
>>> Hi Michael,
>>>
>>> Thanks for the report and for excellent analysis, You are right that
>>> RDMA_NL_LS_F_ERR has the same value as NLM_F_ROOT and it is bad, but
>>> I just think that it is not the final root cause.
>>>
>>> In case of errors, the LS was supposed to send NLMSG_ERROR message and not
>>> overload general nlmsg_flags, which is awful. However I don't know if it is
>>> feasible to fix current implementation without breaking UAPI contract.
>>
>> I agree this is probably something that needs to get followed up on.
>>
>>> In meanwhile, can we implement dummy dumpit functions for the LS,
>>> which reuse ib_nl_is_good_ip_resp?
>>
>> The original code does not call the netlink_dump_start() code for this path, so if we create a dummy dump function we will have to add code to special case this and call it directly.
>>
>> So maybe we could just go back to the original code and call .doit rather than .dump in the non netlink_dump_start() path?
>>
>> i.e.:
>>       if (!(nlh->nlmsg_flags & NLM_F_REQUEST) ||
>>            (index == RDMA_NL_LS && op == RDMA_NL_LS_OP_SET_TIMEOUT)) {
>>                cb.skb = skb;
>>                cb.nlh = nlh;
>>                cb.dump = cb_table[op].dump;
>> -               return cb.dump(skb, &cb);
>> +               return cb.doit(skb, &cb);
>>        } else {
>>                c.dump = cb_table[op].dump;
>>
>>
>>> I prefer this solution over yours, because it doesn't mix LS-specifics with
>>> general decision function and leaves LS anomalies in the LS-relevant code.
>>
>> The problem with this is that the code has to take into consideration the LS anomalies either before the this function is called, or has to deal with them here.
>>
>> The other thought would be to special case the LS stuff before this decision and call the .doit function there (and then return).
> 
> What about such code?
> 
> diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
> index b12e58787c3d..6a7362664876 100644
> --- a/drivers/infiniband/core/netlink.c
> +++ b/drivers/infiniband/core/netlink.c
> @@ -175,6 +175,14 @@ static int rdma_nl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	    !netlink_capable(skb, CAP_NET_ADMIN))
>  		return -EPERM;
> 
> +	/*
> +	 * LS is special because it runs backward communication
> +	 * and it overloads NLM_F_DUMP flag with RDMA_NL_LS_F_ERR
> +	 * So we are calling to .doit before processing .dumpit call.
> +	 */
> +	if (index == RDMA_NL_LS)
> +		return cb_table[op].doit(skb, nlh, extack);
> +
>  	/* FIXME: Convert IWCM to properly handle doit callbacks */
>  	if ((nlh->nlmsg_flags & NLM_F_DUMP) || index == RDMA_NL_RDMA_CM ||
>  	    index == RDMA_NL_IWCM) {
> 
>>
>>> And returning 0 in absence of dumpit function as a response with
>>> NLM_F_DUMP flag is wrong. User should be aware of the fact that
>>> something wrong was with his request.
>>
>> This was the value that the function returns if .dump and .doit are NOT selected, so I thought that this was appropriate.  Should that value (the final return 0) be changed to something different?
> 
> The is_nl_valid() should return "false" If no .dumpit/.doit functions exist.
> In such case, we are supposed to return EINVAL.

My comment here is not specifically related to this issue alone, but to
the overall netlink changes in general.  I just want to make sure people
are aware that this qualifies as a security fix, it *would* need to be
addressed in stable if we weren't still in the same devel cycle that the
overall netlink changes were introduced, and because this exposed the
fact that we could call a routine that does not exist and oops the
kernel, I think we need a quick audit of the netlink code to make sure
there isn't another way for this to happen.  Since this is user input
directing the kernel to jump to a specific callback, this netlink code
must be hardened against intentional attacks (and I haven't looked to
see if this patch is sufficient to do that yet, I'm just trying to set
expectations of what really needs to be done so I can send a complete
pull request to Linus for this).

Cc: Linus to make him aware of the issue and to expect a fix from us
sometime this week.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 6804 bytes --]

On Mon, Oct 23, 2017 at 10:49:24AM -0400, Doug Ledford wrote:
> On 10/23/2017 4:11 AM, Leon Romanovsky wrote:
> > On Fri, Oct 20, 2017 at 05:20:22PM +0000, Ruhl, Michael J wrote:
> >>> -----Original Message-----
> >>> From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> >>> Sent: Friday, October 20, 2017 3:37 AM
> >>> To: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >>> Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> >>> misinterpreted flag
> >>>
> >>> On Thu, Oct 19, 2017 at 05:40:59PM -0400, Michael J. Ruhl wrote:
> >>>> I was playing with the ibacm service and discovered an issue
> >>>> the other day.
> >>>>
> >>>> If no provider library is present (I removed libacmp.so, and the
> >>>> provider keyword in the opts.cfg file is libacmp), when a resolve
> >>>> request is posted, the kernel will crash with the following Oops:
> >>>>
> >>>> Call Trace:
> >>>>  ? netlink_dump+0x12c/0x290
> >>>>  __netlink_dump_start+0x186/0x1f0
> >>>>  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
> >>>>  rdma_nl_rcv+0xdc/0x130 [ib_core]
> >>>>  netlink_unicast+0x181/0x240
> >>>>  netlink_sendmsg+0x2c2/0x3b0
> >>>>  sock_sendmsg+0x38/0x50
> >>>>  SYSC_sendto+0x102/0x190
> >>>>  ? __audit_syscall_entry+0xaf/0x100
> >>>>  ? syscall_trace_enter+0x1d0/0x2b0
> >>>>  ? __audit_syscall_exit+0x209/0x290
> >>>>  SyS_sendto+0xe/0x10
> >>>>  do_syscall_64+0x67/0x1b0
> >>>>  entry_SYSCALL64_slow_path+0x25/0x25
> >>>>
> >>>> The issue is that in rdma_nl_rcv_msg(), the check
> >>>> 'if (flags & NLM_F_DUMP)' is not completely correct.
> >>>>
> >>>> NLM_F_DUMP is two bits NLM_F_ROOT | NLM_F_MATCH.
> >>>>
> >>>> ibacm sends a RDMA_NL_LS response with the RDMA_NL_LS_F_ERR bit set
> >>>> if an error occurs in the service (like no provider being available,
> >>>> or ACM_STATUS_ENODATA, etc.).
> >>>>
> >>>> NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
> >>>>
> >>>> The current code thinks that it sees a NLM_F_DUMP flag and incorrectly calls
> >>>> the .dump() callback.
> >>>
> >>> Hi Michael,
> >>>
> >>> Thanks for the report and for excellent analysis, You are right that
> >>> RDMA_NL_LS_F_ERR has the same value as NLM_F_ROOT and it is bad, but
> >>> I just think that it is not the final root cause.
> >>>
> >>> In case of errors, the LS was supposed to send NLMSG_ERROR message and not
> >>> overload general nlmsg_flags, which is awful. However I don't know if it is
> >>> feasible to fix current implementation without breaking UAPI contract.
> >>
> >> I agree this is probably something that needs to get followed up on.
> >>
> >>> In meanwhile, can we implement dummy dumpit functions for the LS,
> >>> which reuse ib_nl_is_good_ip_resp?
> >>
> >> The original code does not call the netlink_dump_start() code for this path, so if we create a dummy dump function we will have to add code to special case this and call it directly.
> >>
> >> So maybe we could just go back to the original code and call .doit rather than .dump in the non netlink_dump_start() path?
> >>
> >> i.e.:
> >>       if (!(nlh->nlmsg_flags & NLM_F_REQUEST) ||
> >>            (index == RDMA_NL_LS && op == RDMA_NL_LS_OP_SET_TIMEOUT)) {
> >>                cb.skb = skb;
> >>                cb.nlh = nlh;
> >>                cb.dump = cb_table[op].dump;
> >> -               return cb.dump(skb, &cb);
> >> +               return cb.doit(skb, &cb);
> >>        } else {
> >>                c.dump = cb_table[op].dump;
> >>
> >>
> >>> I prefer this solution over yours, because it doesn't mix LS-specifics with
> >>> general decision function and leaves LS anomalies in the LS-relevant code.
> >>
> >> The problem with this is that the code has to take into consideration the LS anomalies either before the this function is called, or has to deal with them here.
> >>
> >> The other thought would be to special case the LS stuff before this decision and call the .doit function there (and then return).
> >
> > What about such code?
> >
> > diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
> > index b12e58787c3d..6a7362664876 100644
> > --- a/drivers/infiniband/core/netlink.c
> > +++ b/drivers/infiniband/core/netlink.c
> > @@ -175,6 +175,14 @@ static int rdma_nl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
> >  	    !netlink_capable(skb, CAP_NET_ADMIN))
> >  		return -EPERM;
> >
> > +	/*
> > +	 * LS is special because it runs backward communication
> > +	 * and it overloads NLM_F_DUMP flag with RDMA_NL_LS_F_ERR
> > +	 * So we are calling to .doit before processing .dumpit call.
> > +	 */
> > +	if (index == RDMA_NL_LS)
> > +		return cb_table[op].doit(skb, nlh, extack);
> > +
> >  	/* FIXME: Convert IWCM to properly handle doit callbacks */
> >  	if ((nlh->nlmsg_flags & NLM_F_DUMP) || index == RDMA_NL_RDMA_CM ||
> >  	    index == RDMA_NL_IWCM) {
> >
> >>
> >>> And returning 0 in absence of dumpit function as a response with
> >>> NLM_F_DUMP flag is wrong. User should be aware of the fact that
> >>> something wrong was with his request.
> >>
> >> This was the value that the function returns if .dump and .doit are NOT selected, so I thought that this was appropriate.  Should that value (the final return 0) be changed to something different?
> >
> > The is_nl_valid() should return "false" If no .dumpit/.doit functions exist.
> > In such case, we are supposed to return EINVAL.
>
> My comment here is not specifically related to this issue alone, but to
> the overall netlink changes in general.  I just want to make sure people
> are aware that this qualifies as a security fix, it *would* need to be
> addressed in stable if we weren't still in the same devel cycle that the
> overall netlink changes were introduced, and because this exposed the
> fact that we could call a routine that does not exist and oops the
> kernel, I think we need a quick audit of the netlink code to make sure
> there isn't another way for this to happen.  Since this is user input
> directing the kernel to jump to a specific callback, this netlink code
> must be hardened against intentional attacks (and I haven't looked to
> see if this patch is sufficient to do that yet, I'm just trying to set
> expectations of what really needs to be done so I can send a complete
> pull request to Linus for this).

Doug,

It has very little related to security here. The RDMA_NL_LS netlink
operations require CAP_NET_ADMIN capability set and it is checked before
calling any callback.

>
> Cc: Linus to make him aware of the issue and to expect a fix from us
> sometime this week.
>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG Key ID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Doug Ledford]

On Mon, 2017-10-23 at 20:12 +0300, Leon Romanovsky wrote:
> On Mon, Oct 23, 2017 at 10:49:24AM -0400, Doug Ledford wrote:
> > On 10/23/2017 4:11 AM, Leon Romanovsky wrote:
> Doug,
> 
> It has very little related to security here. The RDMA_NL_LS netlink
> operations require CAP_NET_ADMIN capability set and it is checked
> before
> calling any callback.

I disagree.  In this particular case, it wasn't a nefarious user, it
was a simple misconfiguration that cause the kernel to oops.  So even
if you have CAP_NET_ADMIN, you still don't want a user space issue to
oops the kernel.  If you simply don't allow it to happen, then whether
the CAP_NET_ADMIN program has been compromised by a black hat user is
irrelevant.  That seems the right way to be to me.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On Mon, Oct 23, 2017 at 01:39:44PM -0400, Doug Ledford wrote:
> On Mon, 2017-10-23 at 20:12 +0300, Leon Romanovsky wrote:
> > On Mon, Oct 23, 2017 at 10:49:24AM -0400, Doug Ledford wrote:
> > > On 10/23/2017 4:11 AM, Leon Romanovsky wrote:
> > Doug,
> >
> > It has very little related to security here. The RDMA_NL_LS netlink
> > operations require CAP_NET_ADMIN capability set and it is checked
> > before
> > calling any callback.
>
> I disagree.  In this particular case, it wasn't a nefarious user, it
> was a simple misconfiguration that cause the kernel to oops.  So even
> if you have CAP_NET_ADMIN, you still don't want a user space issue to
> oops the kernel.  If you simply don't allow it to happen, then whether
> the CAP_NET_ADMIN program has been compromised by a black hat user is
> irrelevant.  That seems the right way to be to me.

OK, fix exists and if you want to call it "security issue", let's call it so.

Despite the fact that root misconfigured the system, root run the program,
root crashed the system, like all over kernel oops we are seeing in linux kernel.

Thanks

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Ruhl, Michael J]

> -----Original Message-----
> From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Monday, October 23, 2017 2:04 PM
> To: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Torvalds, Linus <torvalds@linux-
> foundation.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> misinterpreted flag
> 
> On Mon, Oct 23, 2017 at 01:39:44PM -0400, Doug Ledford wrote:
> > On Mon, 2017-10-23 at 20:12 +0300, Leon Romanovsky wrote:
> > > On Mon, Oct 23, 2017 at 10:49:24AM -0400, Doug Ledford wrote:
> > > > On 10/23/2017 4:11 AM, Leon Romanovsky wrote:
> > > Doug,
> > >
> > > It has very little related to security here. The RDMA_NL_LS netlink
> > > operations require CAP_NET_ADMIN capability set and it is checked
> > > before
> > > calling any callback.
> >
> > I disagree.  In this particular case, it wasn't a nefarious user, it
> > was a simple misconfiguration that cause the kernel to oops.  So even
> > if you have CAP_NET_ADMIN, you still don't want a user space issue to
> > oops the kernel.  If you simply don't allow it to happen, then whether
> > the CAP_NET_ADMIN program has been compromised by a black hat user is
> > irrelevant.  That seems the right way to be to me.
> 
> OK, fix exists and if you want to call it "security issue", let's call it so.
> 
> Despite the fact that root misconfigured the system, root run the program,
> root crashed the system, like all over kernel oops we are seeing in linux kernel.
> 
> Thanks

I did repeat this once without the misconfiguration.

The scenario was that I had that a local (ibacm client 0) did a look up, got an error, and the system crashed.

I have been trying to remember what I did, but haven't repeated it a second time.  I will see if I can figure out how to make it happen again.

M


 
> >
> > --
> > Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >     GPG KeyID: B826A3330E572FDD
> >     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2415 bytes --]

On Mon, Oct 23, 2017 at 06:19:11PM +0000, Ruhl, Michael J wrote:
> > -----Original Message-----
> > From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> > Sent: Monday, October 23, 2017 2:04 PM
> > To: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Torvalds, Linus <torvalds@linux-
> > foundation.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> > misinterpreted flag
> >
> > On Mon, Oct 23, 2017 at 01:39:44PM -0400, Doug Ledford wrote:
> > > On Mon, 2017-10-23 at 20:12 +0300, Leon Romanovsky wrote:
> > > > On Mon, Oct 23, 2017 at 10:49:24AM -0400, Doug Ledford wrote:
> > > > > On 10/23/2017 4:11 AM, Leon Romanovsky wrote:
> > > > Doug,
> > > >
> > > > It has very little related to security here. The RDMA_NL_LS netlink
> > > > operations require CAP_NET_ADMIN capability set and it is checked
> > > > before
> > > > calling any callback.
> > >
> > > I disagree.  In this particular case, it wasn't a nefarious user, it
> > > was a simple misconfiguration that cause the kernel to oops.  So even
> > > if you have CAP_NET_ADMIN, you still don't want a user space issue to
> > > oops the kernel.  If you simply don't allow it to happen, then whether
> > > the CAP_NET_ADMIN program has been compromised by a black hat user is
> > > irrelevant.  That seems the right way to be to me.
> >
> > OK, fix exists and if you want to call it "security issue", let's call it so.
> >
> > Despite the fact that root misconfigured the system, root run the program,
> > root crashed the system, like all over kernel oops we are seeing in linux kernel.
> >
> > Thanks
>
> I did repeat this once without the misconfiguration.
>
> The scenario was that I had that a local (ibacm client 0) did a look up, got an error, and the system crashed.
>
> I have been trying to remember what I did, but haven't repeated it a second time.  I will see if I can figure out how to make it happen again.

Actually, you need to cause an error from ibacm side.

Just send a fix with stable tag.

Thanks for doing that.

>
> M
>
>
>
> > >
> > > --
> > > Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >     GPG KeyID: B826A3330E572FDD
> > >     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
> > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Ruhl, Michael J]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
> Sent: Monday, October 23, 2017 2:25 PM
> To: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; Torvalds, Linus <torvalds@linux-
> foundation.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> misinterpreted flag
> >
> > I did repeat this once without the misconfiguration.
> >
> > The scenario was that I had that a local (ibacm client 0) did a look up, got an
> error, and the system crashed.
> >
> > I have been trying to remember what I did, but haven't repeated it a second
> time.  I will see if I can figure out how to make it happen again.
> 
> Actually, you need to cause an error from ibacm side.
> 
> Just send a fix with stable tag.
> 
> Thanks for doing that.

The patch is working its way through our internal process.  Once that is done, I will post it.

Thanks,

Mike

> 
> >
> > M
> >
> >
> >
> > > >
> > > > --
> > > > Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > >     GPG KeyID: B826A3330E572FDD
> > > >     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57
> 2FDD
> > > >
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Michael J. Ruhl]

From: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

rdma_nl_rcv_msg() checks to see if it should use the .dump() callback
or the .doit() callback.  The check is done with this check:

if (flags & NLM_F_DUMP) ...

The NLM_F_DUMP flag is two bits (NLM_F_ROOT | NLM_F_MATCH).

When an RDMA_NL_LS message (response) is received, the bit used for
indicating an error is the same bit as NLM_F_ROOT.

NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.

ibacm sends a response with the RDMA_NL_LS_F_ERR bit set if an error
occurs in the service.  The current code then misinterprets the
NLM_F_DUMP bit and trys to call the .dump() callback.

If the .dump() callback for the specified request is not available
(which is true for the RDMA_NL_LS messages) the following Oops occurs:

[ 4555.960256] BUG: unable to handle kernel NULL pointer dereference at
   (null)
[ 4555.969046] IP:           (null)
[ 4555.972664] PGD 10543f1067 P4D 10543f1067 PUD 1033f93067 PMD 0
[ 4555.979287] Oops: 0010 [#1] SMP
[ 4555.982809] Modules linked in: rpcrdma ib_isert iscsi_target_mod
target_core_mod ib_iser libiscsi scsi_transport_iscsi ib_ipoib rdma_ucm ib_ucm
ib_uverbs ib_umad rdma_cm ib_cm iw_cm dm_mirror dm_region_hash dm_log dm_mod
dax sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass
crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_simd
glue_helper cryptd hfi1 rdmavt iTCO_wdt iTCO_vendor_support ib_core mei_me
lpc_ich pcspkr mei ioatdma sg shpchp i2c_i801 mfd_core wmi ipmi_si ipmi_devintf
ipmi_msghandler acpi_power_meter acpi_pad nfsd auth_rpcgss nfs_acl lockd grace
sunrpc ip_tables ext4 mbcache jbd2 sd_mod mgag200 drm_kms_helper syscopyarea
sysfillrect sysimgblt fb_sys_fops ttm igb ahci crc32c_intel ptp libahci
pps_core drm dca libata i2c_algo_bit i2c_core
[ 4556.061190] CPU: 54 PID: 9841 Comm: ibacm Tainted: G          I
4.14.0-rc2+ #6
[ 4556.069667] Hardware name: Intel Corporation S2600WT2/S2600WT2, BIOS
SE5C610.86B.01.01.0008.021120151325 02/11/2015
[ 4556.081339] task: ffff880855f42d00 task.stack: ffffc900246b4000
[ 4556.087967] RIP: 0010:          (null)
[ 4556.092166] RSP: 0018:ffffc900246b7bc8 EFLAGS: 00010246
[ 4556.098018] RAX: ffffffff81dbe9e0 RBX: ffff881058bb1000 RCX:
0000000000000000
[ 4556.105997] RDX: 0000000000001100 RSI: ffff881058bb1320 RDI:
ffff881056362000
[ 4556.113984] RBP: ffffc900246b7bf8 R08: 0000000000000ec0 R09:
0000000000001100
[ 4556.121971] R10: ffff8810573a5000 R11: 0000000000000000 R12:
ffff881056362000
[ 4556.129957] R13: 0000000000000ec0 R14: ffff881058bb1320 R15:
0000000000000ec0
[ 4556.137945] FS:  00007fe0ba5a38c0(0000) GS:ffff88105f080000(0000)
knlGS:0000000000000000
[ 4556.147000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4556.153433] CR2: 0000000000000000 CR3: 0000001056f5d003 CR4:
00000000001606e0
[ 4556.161419] Call Trace:
[ 4556.164167]  ? netlink_dump+0x12c/0x290
[ 4556.168468]  __netlink_dump_start+0x186/0x1f0
[ 4556.173357]  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
[ 4556.178724]  rdma_nl_rcv+0xdc/0x130 [ib_core]
[ 4556.183604]  netlink_unicast+0x181/0x240
[ 4556.187998]  netlink_sendmsg+0x2c2/0x3b0
[ 4556.192392]  sock_sendmsg+0x38/0x50
[ 4556.196299]  SYSC_sendto+0x102/0x190
[ 4556.200308]  ? __audit_syscall_entry+0xaf/0x100
[ 4556.205387]  ? syscall_trace_enter+0x1d0/0x2b0
[ 4556.210366]  ? __audit_syscall_exit+0x209/0x290
[ 4556.215442]  SyS_sendto+0xe/0x10
[ 4556.219060]  do_syscall_64+0x67/0x1b0
[ 4556.223165]  entry_SYSCALL64_slow_path+0x25/0x25
[ 4556.228328] RIP: 0033:0x7fe0b9db2a63
[ 4556.232333] RSP: 002b:00007ffc55edc260 EFLAGS: 00000293 ORIG_RAX:
000000000000002c
[ 4556.240808] RAX: ffffffffffffffda RBX: 0000000000000010 RCX:
00007fe0b9db2a63
[ 4556.248796] RDX: 0000000000000010 RSI: 00007ffc55edc280 RDI:
000000000000000d
[ 4556.256782] RBP: 00007ffc55edc670 R08: 00007ffc55edc270 R09:
000000000000000c
[ 4556.265321] R10: 0000000000000000 R11: 0000000000000293 R12:
00007ffc55edc280
[ 4556.273846] R13: 000000000260b400 R14: 000000000000000d R15:
0000000000000001
[ 4556.282368] Code:  Bad RIP value.
[ 4556.286629] RIP:           (null) RSP: ffffc900246b7bc8
[ 4556.293013] CR2: 0000000000000000
[ 4556.297292] ---[ end trace 8d67abcfd10ec209 ]---
[ 4556.305465] Kernel panic - not syncing: Fatal exception
[ 4556.313786] Kernel Offset: disabled
[ 4556.321563] ---[ end Kernel panic - not syncing: Fatal exception
[ 4556.328960] ------------[ cut here ]------------

Special case RDMA_NL_LS response messages to call the appropriate
callback.

Additionally, make sure that the .dump() callback is not NULL
before calling it.

Fixes: 647c75ac59a48a54 ("RDMA/netlink: Convert LS to doit callback")
Reviewed-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Kaike Wan <kaike.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Alex Estrin <alex.estrin-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/core/netlink.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
index b12e587..1fb72c3 100644
--- a/drivers/infiniband/core/netlink.c
+++ b/drivers/infiniband/core/netlink.c
@@ -175,13 +175,24 @@ static int rdma_nl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	    !netlink_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
+	/*
+	 * LS responses overload the 0x100 (NLM_F_ROOT) flag.  Don't
+	 * mistakenly call the .dump() function.
+	 */
+	if (index == RDMA_NL_LS) {
+		if (cb_table[op].doit)
+			return cb_table[op].doit(skb, nlh, extack);
+		return -EINVAL;
+	}
 	/* FIXME: Convert IWCM to properly handle doit callbacks */
 	if ((nlh->nlmsg_flags & NLM_F_DUMP) || index == RDMA_NL_RDMA_CM ||
 	    index == RDMA_NL_IWCM) {
 		struct netlink_dump_control c = {
 			.dump = cb_table[op].dump,
 		};
-		return netlink_dump_start(nls, skb, nlh, &c);
+		if (c.dump)
+			return netlink_dump_start(nls, skb, nlh, &c);
+		return -EINVAL;
 	}
 
 	if (cb_table[op].doit)

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 5013 bytes --]

On Tue, Oct 24, 2017 at 08:41:01AM -0400, Michael J. Ruhl wrote:
> From: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> rdma_nl_rcv_msg() checks to see if it should use the .dump() callback
> or the .doit() callback.  The check is done with this check:
>
> if (flags & NLM_F_DUMP) ...
>
> The NLM_F_DUMP flag is two bits (NLM_F_ROOT | NLM_F_MATCH).
>
> When an RDMA_NL_LS message (response) is received, the bit used for
> indicating an error is the same bit as NLM_F_ROOT.
>
> NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
>
> ibacm sends a response with the RDMA_NL_LS_F_ERR bit set if an error
> occurs in the service.  The current code then misinterprets the
> NLM_F_DUMP bit and trys to call the .dump() callback.
>
> If the .dump() callback for the specified request is not available
> (which is true for the RDMA_NL_LS messages) the following Oops occurs:
>
> [ 4555.960256] BUG: unable to handle kernel NULL pointer dereference at
>    (null)
> [ 4555.969046] IP:           (null)
> [ 4555.972664] PGD 10543f1067 P4D 10543f1067 PUD 1033f93067 PMD 0
> [ 4555.979287] Oops: 0010 [#1] SMP
> [ 4555.982809] Modules linked in: rpcrdma ib_isert iscsi_target_mod
> target_core_mod ib_iser libiscsi scsi_transport_iscsi ib_ipoib rdma_ucm ib_ucm
> ib_uverbs ib_umad rdma_cm ib_cm iw_cm dm_mirror dm_region_hash dm_log dm_mod
> dax sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass
> crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_simd
> glue_helper cryptd hfi1 rdmavt iTCO_wdt iTCO_vendor_support ib_core mei_me
> lpc_ich pcspkr mei ioatdma sg shpchp i2c_i801 mfd_core wmi ipmi_si ipmi_devintf
> ipmi_msghandler acpi_power_meter acpi_pad nfsd auth_rpcgss nfs_acl lockd grace
> sunrpc ip_tables ext4 mbcache jbd2 sd_mod mgag200 drm_kms_helper syscopyarea
> sysfillrect sysimgblt fb_sys_fops ttm igb ahci crc32c_intel ptp libahci
> pps_core drm dca libata i2c_algo_bit i2c_core
> [ 4556.061190] CPU: 54 PID: 9841 Comm: ibacm Tainted: G          I
> 4.14.0-rc2+ #6
> [ 4556.069667] Hardware name: Intel Corporation S2600WT2/S2600WT2, BIOS
> SE5C610.86B.01.01.0008.021120151325 02/11/2015
> [ 4556.081339] task: ffff880855f42d00 task.stack: ffffc900246b4000
> [ 4556.087967] RIP: 0010:          (null)
> [ 4556.092166] RSP: 0018:ffffc900246b7bc8 EFLAGS: 00010246
> [ 4556.098018] RAX: ffffffff81dbe9e0 RBX: ffff881058bb1000 RCX:
> 0000000000000000
> [ 4556.105997] RDX: 0000000000001100 RSI: ffff881058bb1320 RDI:
> ffff881056362000
> [ 4556.113984] RBP: ffffc900246b7bf8 R08: 0000000000000ec0 R09:
> 0000000000001100
> [ 4556.121971] R10: ffff8810573a5000 R11: 0000000000000000 R12:
> ffff881056362000
> [ 4556.129957] R13: 0000000000000ec0 R14: ffff881058bb1320 R15:
> 0000000000000ec0
> [ 4556.137945] FS:  00007fe0ba5a38c0(0000) GS:ffff88105f080000(0000)
> knlGS:0000000000000000
> [ 4556.147000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 4556.153433] CR2: 0000000000000000 CR3: 0000001056f5d003 CR4:
> 00000000001606e0
> [ 4556.161419] Call Trace:
> [ 4556.164167]  ? netlink_dump+0x12c/0x290
> [ 4556.168468]  __netlink_dump_start+0x186/0x1f0
> [ 4556.173357]  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
> [ 4556.178724]  rdma_nl_rcv+0xdc/0x130 [ib_core]
> [ 4556.183604]  netlink_unicast+0x181/0x240
> [ 4556.187998]  netlink_sendmsg+0x2c2/0x3b0
> [ 4556.192392]  sock_sendmsg+0x38/0x50
> [ 4556.196299]  SYSC_sendto+0x102/0x190
> [ 4556.200308]  ? __audit_syscall_entry+0xaf/0x100
> [ 4556.205387]  ? syscall_trace_enter+0x1d0/0x2b0
> [ 4556.210366]  ? __audit_syscall_exit+0x209/0x290
> [ 4556.215442]  SyS_sendto+0xe/0x10
> [ 4556.219060]  do_syscall_64+0x67/0x1b0
> [ 4556.223165]  entry_SYSCALL64_slow_path+0x25/0x25
> [ 4556.228328] RIP: 0033:0x7fe0b9db2a63
> [ 4556.232333] RSP: 002b:00007ffc55edc260 EFLAGS: 00000293 ORIG_RAX:
> 000000000000002c
> [ 4556.240808] RAX: ffffffffffffffda RBX: 0000000000000010 RCX:
> 00007fe0b9db2a63
> [ 4556.248796] RDX: 0000000000000010 RSI: 00007ffc55edc280 RDI:
> 000000000000000d
> [ 4556.256782] RBP: 00007ffc55edc670 R08: 00007ffc55edc270 R09:
> 000000000000000c
> [ 4556.265321] R10: 0000000000000000 R11: 0000000000000293 R12:
> 00007ffc55edc280
> [ 4556.273846] R13: 000000000260b400 R14: 000000000000000d R15:
> 0000000000000001
> [ 4556.282368] Code:  Bad RIP value.
> [ 4556.286629] RIP:           (null) RSP: ffffc900246b7bc8
> [ 4556.293013] CR2: 0000000000000000
> [ 4556.297292] ---[ end trace 8d67abcfd10ec209 ]---
> [ 4556.305465] Kernel panic - not syncing: Fatal exception
> [ 4556.313786] Kernel Offset: disabled
> [ 4556.321563] ---[ end Kernel panic - not syncing: Fatal exception
> [ 4556.328960] ------------[ cut here ]------------
>
> Special case RDMA_NL_LS response messages to call the appropriate
> callback.
>
> Additionally, make sure that the .dump() callback is not NULL
> before calling it.

Please don't add them here, it is dead code in wrong place.
First it is unachievable paths, and second they need to be checked
in is_nl_valid() function.

Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Shiraz Saleem]

On Tue, Oct 24, 2017 at 08:41:01AM -0400, Michael J. Ruhl wrote:
> From: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> ---
>  drivers/infiniband/core/netlink.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
> index b12e587..1fb72c3 100644
> --- a/drivers/infiniband/core/netlink.c
> +++ b/drivers/infiniband/core/netlink.c
> @@ -175,13 +175,24 @@ static int rdma_nl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	    !netlink_capable(skb, CAP_NET_ADMIN))
>  		return -EPERM;
>  
> +	/*
> +	 * LS responses overload the 0x100 (NLM_F_ROOT) flag.  Don't
> +	 * mistakenly call the .dump() function.
> +	 */
> +	if (index == RDMA_NL_LS) {
> +		if (cb_table[op].doit)
> +			return cb_table[op].doit(skb, nlh, extack);
> +		return -EINVAL;
> +	}
>  	/* FIXME: Convert IWCM to properly handle doit callbacks */
>  	if ((nlh->nlmsg_flags & NLM_F_DUMP) || index == RDMA_NL_RDMA_CM ||
>  	    index == RDMA_NL_IWCM) {
>  		struct netlink_dump_control c = {
>  			.dump = cb_table[op].dump,
>  		};
> -		return netlink_dump_start(nls, skb, nlh, &c);
> +		if (c.dump)
> +			return netlink_dump_start(nls, skb, nlh, &c);
> +		return -EINVAL;
>  	}
>  
>  	if (cb_table[op].doit)
>

Do you neccessarily need the non-null checks for cb_table[op].doit and c.dump?

Otherwise, looks good.

Reviewed-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Ruhl, Michael J]

> -----Original Message-----
> From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Tuesday, October 24, 2017 10:42 AM
> To: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> misinterpreted flag
> 
> On Tue, Oct 24, 2017 at 08:41:01AM -0400, Michael J. Ruhl wrote:
> > From: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >
> > rdma_nl_rcv_msg() checks to see if it should use the .dump() callback
> > or the .doit() callback.  The check is done with this check:
> >
> > if (flags & NLM_F_DUMP) ...
> >
> > The NLM_F_DUMP flag is two bits (NLM_F_ROOT | NLM_F_MATCH).
> >
> > When an RDMA_NL_LS message (response) is received, the bit used for
> > indicating an error is the same bit as NLM_F_ROOT.
> >
> > NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
> >
> > ibacm sends a response with the RDMA_NL_LS_F_ERR bit set if an error
> > occurs in the service.  The current code then misinterprets the
> > NLM_F_DUMP bit and trys to call the .dump() callback.
> >
> > If the .dump() callback for the specified request is not available
> > (which is true for the RDMA_NL_LS messages) the following Oops occurs:
> >
> > [ 4555.960256] BUG: unable to handle kernel NULL pointer dereference at
> >    (null)
> > [ 4555.969046] IP:           (null)
> > [ 4555.972664] PGD 10543f1067 P4D 10543f1067 PUD 1033f93067 PMD 0
> > [ 4555.979287] Oops: 0010 [#1] SMP
> > [ 4555.982809] Modules linked in: rpcrdma ib_isert iscsi_target_mod
> > target_core_mod ib_iser libiscsi scsi_transport_iscsi ib_ipoib rdma_ucm
> ib_ucm
> > ib_uverbs ib_umad rdma_cm ib_cm iw_cm dm_mirror dm_region_hash
> dm_log dm_mod
> > dax sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm
> irqbypass
> > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel
> crypto_simd
> > glue_helper cryptd hfi1 rdmavt iTCO_wdt iTCO_vendor_support ib_core
> mei_me
> > lpc_ich pcspkr mei ioatdma sg shpchp i2c_i801 mfd_core wmi ipmi_si
> ipmi_devintf
> > ipmi_msghandler acpi_power_meter acpi_pad nfsd auth_rpcgss nfs_acl lockd
> grace
> > sunrpc ip_tables ext4 mbcache jbd2 sd_mod mgag200 drm_kms_helper
> syscopyarea
> > sysfillrect sysimgblt fb_sys_fops ttm igb ahci crc32c_intel ptp libahci
> > pps_core drm dca libata i2c_algo_bit i2c_core
> > [ 4556.061190] CPU: 54 PID: 9841 Comm: ibacm Tainted: G          I
> > 4.14.0-rc2+ #6
> > [ 4556.069667] Hardware name: Intel Corporation S2600WT2/S2600WT2,
> BIOS
> > SE5C610.86B.01.01.0008.021120151325 02/11/2015
> > [ 4556.081339] task: ffff880855f42d00 task.stack: ffffc900246b4000
> > [ 4556.087967] RIP: 0010:          (null)
> > [ 4556.092166] RSP: 0018:ffffc900246b7bc8 EFLAGS: 00010246
> > [ 4556.098018] RAX: ffffffff81dbe9e0 RBX: ffff881058bb1000 RCX:
> > 0000000000000000
> > [ 4556.105997] RDX: 0000000000001100 RSI: ffff881058bb1320 RDI:
> > ffff881056362000
> > [ 4556.113984] RBP: ffffc900246b7bf8 R08: 0000000000000ec0 R09:
> > 0000000000001100
> > [ 4556.121971] R10: ffff8810573a5000 R11: 0000000000000000 R12:
> > ffff881056362000
> > [ 4556.129957] R13: 0000000000000ec0 R14: ffff881058bb1320 R15:
> > 0000000000000ec0
> > [ 4556.137945] FS:  00007fe0ba5a38c0(0000) GS:ffff88105f080000(0000)
> > knlGS:0000000000000000
> > [ 4556.147000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 4556.153433] CR2: 0000000000000000 CR3: 0000001056f5d003 CR4:
> > 00000000001606e0
> > [ 4556.161419] Call Trace:
> > [ 4556.164167]  ? netlink_dump+0x12c/0x290
> > [ 4556.168468]  __netlink_dump_start+0x186/0x1f0
> > [ 4556.173357]  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
> > [ 4556.178724]  rdma_nl_rcv+0xdc/0x130 [ib_core]
> > [ 4556.183604]  netlink_unicast+0x181/0x240
> > [ 4556.187998]  netlink_sendmsg+0x2c2/0x3b0
> > [ 4556.192392]  sock_sendmsg+0x38/0x50
> > [ 4556.196299]  SYSC_sendto+0x102/0x190
> > [ 4556.200308]  ? __audit_syscall_entry+0xaf/0x100
> > [ 4556.205387]  ? syscall_trace_enter+0x1d0/0x2b0
> > [ 4556.210366]  ? __audit_syscall_exit+0x209/0x290
> > [ 4556.215442]  SyS_sendto+0xe/0x10
> > [ 4556.219060]  do_syscall_64+0x67/0x1b0
> > [ 4556.223165]  entry_SYSCALL64_slow_path+0x25/0x25
> > [ 4556.228328] RIP: 0033:0x7fe0b9db2a63
> > [ 4556.232333] RSP: 002b:00007ffc55edc260 EFLAGS: 00000293 ORIG_RAX:
> > 000000000000002c
> > [ 4556.240808] RAX: ffffffffffffffda RBX: 0000000000000010 RCX:
> > 00007fe0b9db2a63
> > [ 4556.248796] RDX: 0000000000000010 RSI: 00007ffc55edc280 RDI:
> > 000000000000000d
> > [ 4556.256782] RBP: 00007ffc55edc670 R08: 00007ffc55edc270 R09:
> > 000000000000000c
> > [ 4556.265321] R10: 0000000000000000 R11: 0000000000000293 R12:
> > 00007ffc55edc280
> > [ 4556.273846] R13: 000000000260b400 R14: 000000000000000d R15:
> > 0000000000000001
> > [ 4556.282368] Code:  Bad RIP value.
> > [ 4556.286629] RIP:           (null) RSP: ffffc900246b7bc8
> > [ 4556.293013] CR2: 0000000000000000
> > [ 4556.297292] ---[ end trace 8d67abcfd10ec209 ]---
> > [ 4556.305465] Kernel panic - not syncing: Fatal exception
> > [ 4556.313786] Kernel Offset: disabled
> > [ 4556.321563] ---[ end Kernel panic - not syncing: Fatal exception
> > [ 4556.328960] ------------[ cut here ]------------
> >
> > Special case RDMA_NL_LS response messages to call the appropriate
> > callback.
> >
> > Additionally, make sure that the .dump() callback is not NULL
> > before calling it.
> 
> Please don't add them here, it is dead code in wrong place.
> First it is unachievable paths, and second they need to be checked
> in is_nl_valid() function.

The check in is_nl_valid() is for .doit OR .done.  There is no context based on the index.
So if a specific index has one or the other is_nl_valid is true.

This is what caused the Oops (index only had .doit callbacks, but .dump path
was chosen).  So I don't think that these are unachievable paths.

The is_nl_valid() function is incomplete.  This shows that packets can be crafted
to cause the wrong path to be taken, and if there is no callback function we will
have the same issue.  Specific index validation is needed, and should probably be
addressed with a different patch.

Thanks,

M
> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 6837 bytes --]

On Tue, Oct 24, 2017 at 02:52:31PM +0000, Ruhl, Michael J wrote:
> > -----Original Message-----
> > From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> > Sent: Tuesday, October 24, 2017 10:42 AM
> > To: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> > misinterpreted flag
> >
> > On Tue, Oct 24, 2017 at 08:41:01AM -0400, Michael J. Ruhl wrote:
> > > From: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >
> > > rdma_nl_rcv_msg() checks to see if it should use the .dump() callback
> > > or the .doit() callback.  The check is done with this check:
> > >
> > > if (flags & NLM_F_DUMP) ...
> > >
> > > The NLM_F_DUMP flag is two bits (NLM_F_ROOT | NLM_F_MATCH).
> > >
> > > When an RDMA_NL_LS message (response) is received, the bit used for
> > > indicating an error is the same bit as NLM_F_ROOT.
> > >
> > > NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
> > >
> > > ibacm sends a response with the RDMA_NL_LS_F_ERR bit set if an error
> > > occurs in the service.  The current code then misinterprets the
> > > NLM_F_DUMP bit and trys to call the .dump() callback.
> > >
> > > If the .dump() callback for the specified request is not available
> > > (which is true for the RDMA_NL_LS messages) the following Oops occurs:
> > >
> > > [ 4555.960256] BUG: unable to handle kernel NULL pointer dereference at
> > >    (null)
> > > [ 4555.969046] IP:           (null)
> > > [ 4555.972664] PGD 10543f1067 P4D 10543f1067 PUD 1033f93067 PMD 0
> > > [ 4555.979287] Oops: 0010 [#1] SMP
> > > [ 4555.982809] Modules linked in: rpcrdma ib_isert iscsi_target_mod
> > > target_core_mod ib_iser libiscsi scsi_transport_iscsi ib_ipoib rdma_ucm
> > ib_ucm
> > > ib_uverbs ib_umad rdma_cm ib_cm iw_cm dm_mirror dm_region_hash
> > dm_log dm_mod
> > > dax sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm
> > irqbypass
> > > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel
> > crypto_simd
> > > glue_helper cryptd hfi1 rdmavt iTCO_wdt iTCO_vendor_support ib_core
> > mei_me
> > > lpc_ich pcspkr mei ioatdma sg shpchp i2c_i801 mfd_core wmi ipmi_si
> > ipmi_devintf
> > > ipmi_msghandler acpi_power_meter acpi_pad nfsd auth_rpcgss nfs_acl lockd
> > grace
> > > sunrpc ip_tables ext4 mbcache jbd2 sd_mod mgag200 drm_kms_helper
> > syscopyarea
> > > sysfillrect sysimgblt fb_sys_fops ttm igb ahci crc32c_intel ptp libahci
> > > pps_core drm dca libata i2c_algo_bit i2c_core
> > > [ 4556.061190] CPU: 54 PID: 9841 Comm: ibacm Tainted: G          I
> > > 4.14.0-rc2+ #6
> > > [ 4556.069667] Hardware name: Intel Corporation S2600WT2/S2600WT2,
> > BIOS
> > > SE5C610.86B.01.01.0008.021120151325 02/11/2015
> > > [ 4556.081339] task: ffff880855f42d00 task.stack: ffffc900246b4000
> > > [ 4556.087967] RIP: 0010:          (null)
> > > [ 4556.092166] RSP: 0018:ffffc900246b7bc8 EFLAGS: 00010246
> > > [ 4556.098018] RAX: ffffffff81dbe9e0 RBX: ffff881058bb1000 RCX:
> > > 0000000000000000
> > > [ 4556.105997] RDX: 0000000000001100 RSI: ffff881058bb1320 RDI:
> > > ffff881056362000
> > > [ 4556.113984] RBP: ffffc900246b7bf8 R08: 0000000000000ec0 R09:
> > > 0000000000001100
> > > [ 4556.121971] R10: ffff8810573a5000 R11: 0000000000000000 R12:
> > > ffff881056362000
> > > [ 4556.129957] R13: 0000000000000ec0 R14: ffff881058bb1320 R15:
> > > 0000000000000ec0
> > > [ 4556.137945] FS:  00007fe0ba5a38c0(0000) GS:ffff88105f080000(0000)
> > > knlGS:0000000000000000
> > > [ 4556.147000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [ 4556.153433] CR2: 0000000000000000 CR3: 0000001056f5d003 CR4:
> > > 00000000001606e0
> > > [ 4556.161419] Call Trace:
> > > [ 4556.164167]  ? netlink_dump+0x12c/0x290
> > > [ 4556.168468]  __netlink_dump_start+0x186/0x1f0
> > > [ 4556.173357]  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
> > > [ 4556.178724]  rdma_nl_rcv+0xdc/0x130 [ib_core]
> > > [ 4556.183604]  netlink_unicast+0x181/0x240
> > > [ 4556.187998]  netlink_sendmsg+0x2c2/0x3b0
> > > [ 4556.192392]  sock_sendmsg+0x38/0x50
> > > [ 4556.196299]  SYSC_sendto+0x102/0x190
> > > [ 4556.200308]  ? __audit_syscall_entry+0xaf/0x100
> > > [ 4556.205387]  ? syscall_trace_enter+0x1d0/0x2b0
> > > [ 4556.210366]  ? __audit_syscall_exit+0x209/0x290
> > > [ 4556.215442]  SyS_sendto+0xe/0x10
> > > [ 4556.219060]  do_syscall_64+0x67/0x1b0
> > > [ 4556.223165]  entry_SYSCALL64_slow_path+0x25/0x25
> > > [ 4556.228328] RIP: 0033:0x7fe0b9db2a63
> > > [ 4556.232333] RSP: 002b:00007ffc55edc260 EFLAGS: 00000293 ORIG_RAX:
> > > 000000000000002c
> > > [ 4556.240808] RAX: ffffffffffffffda RBX: 0000000000000010 RCX:
> > > 00007fe0b9db2a63
> > > [ 4556.248796] RDX: 0000000000000010 RSI: 00007ffc55edc280 RDI:
> > > 000000000000000d
> > > [ 4556.256782] RBP: 00007ffc55edc670 R08: 00007ffc55edc270 R09:
> > > 000000000000000c
> > > [ 4556.265321] R10: 0000000000000000 R11: 0000000000000293 R12:
> > > 00007ffc55edc280
> > > [ 4556.273846] R13: 000000000260b400 R14: 000000000000000d R15:
> > > 0000000000000001
> > > [ 4556.282368] Code:  Bad RIP value.
> > > [ 4556.286629] RIP:           (null) RSP: ffffc900246b7bc8
> > > [ 4556.293013] CR2: 0000000000000000
> > > [ 4556.297292] ---[ end trace 8d67abcfd10ec209 ]---
> > > [ 4556.305465] Kernel panic - not syncing: Fatal exception
> > > [ 4556.313786] Kernel Offset: disabled
> > > [ 4556.321563] ---[ end Kernel panic - not syncing: Fatal exception
> > > [ 4556.328960] ------------[ cut here ]------------
> > >
> > > Special case RDMA_NL_LS response messages to call the appropriate
> > > callback.
> > >
> > > Additionally, make sure that the .dump() callback is not NULL
> > > before calling it.
> >
> > Please don't add them here, it is dead code in wrong place.
> > First it is unachievable paths, and second they need to be checked
> > in is_nl_valid() function.
>
> The check in is_nl_valid() is for .doit OR .done.  There is no context based on the index.
> So if a specific index has one or the other is_nl_valid is true.
>
> This is what caused the Oops (index only had .doit callbacks, but .dump path
> was chosen).  So I don't think that these are unachievable paths.
>
> The is_nl_valid() function is incomplete.  This shows that packets can be crafted
> to cause the wrong path to be taken, and if there is no callback function we will
> have the same issue.  Specific index validation is needed, and should probably be
> addressed with a different patch.

.doit exists for RDMA_NL_LS and for other .dumpit exists, so no actual
check is needed.

if you want to be on the safe side, please extend is_nl_valid(). IMHO,
it will keep code cleaner, the checks in one localized place and execution
is not mixed with the validity checks.

Thanks

>
> Thanks,
>
> M
> > Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Ruhl, Michael J]

> -----Original Message-----
> From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Tuesday, October 24, 2017 11:20 AM
> To: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> misinterpreted flag
> 
> On Tue, Oct 24, 2017 at 02:52:31PM +0000, Ruhl, Michael J wrote:
> > > -----Original Message-----
> > > From: Leon Romanovsky [mailto:leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> > > Sent: Tuesday, October 24, 2017 10:42 AM
> > > To: Ruhl, Michael J <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> > > misinterpreted flag
> > >
> > > On Tue, Oct 24, 2017 at 08:41:01AM -0400, Michael J. Ruhl wrote:
> > > > From: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > >
> > > > rdma_nl_rcv_msg() checks to see if it should use the .dump() callback
> > > > or the .doit() callback.  The check is done with this check:
> > > >
> > > > if (flags & NLM_F_DUMP) ...
> > > >
> > > > The NLM_F_DUMP flag is two bits (NLM_F_ROOT | NLM_F_MATCH).
> > > >
> > > > When an RDMA_NL_LS message (response) is received, the bit used for
> > > > indicating an error is the same bit as NLM_F_ROOT.
> > > >
> > > > NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.
> > > >
> > > > ibacm sends a response with the RDMA_NL_LS_F_ERR bit set if an error
> > > > occurs in the service.  The current code then misinterprets the
> > > > NLM_F_DUMP bit and trys to call the .dump() callback.
> > > >
> > > > If the .dump() callback for the specified request is not available
> > > > (which is true for the RDMA_NL_LS messages) the following Oops occurs:
> > > >
> > > > [ 4555.960256] BUG: unable to handle kernel NULL pointer dereference at
> > > >    (null)
> > > > [ 4555.969046] IP:           (null)
> > > > [ 4555.972664] PGD 10543f1067 P4D 10543f1067 PUD 1033f93067 PMD 0
> > > > [ 4555.979287] Oops: 0010 [#1] SMP
> > > > [ 4555.982809] Modules linked in: rpcrdma ib_isert iscsi_target_mod
> > > > target_core_mod ib_iser libiscsi scsi_transport_iscsi ib_ipoib rdma_ucm
> > > ib_ucm
> > > > ib_uverbs ib_umad rdma_cm ib_cm iw_cm dm_mirror dm_region_hash
> > > dm_log dm_mod
> > > > dax sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm
> > > irqbypass
> > > > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel
> > > crypto_simd
> > > > glue_helper cryptd hfi1 rdmavt iTCO_wdt iTCO_vendor_support ib_core
> > > mei_me
> > > > lpc_ich pcspkr mei ioatdma sg shpchp i2c_i801 mfd_core wmi ipmi_si
> > > ipmi_devintf
> > > > ipmi_msghandler acpi_power_meter acpi_pad nfsd auth_rpcgss nfs_acl
> lockd
> > > grace
> > > > sunrpc ip_tables ext4 mbcache jbd2 sd_mod mgag200 drm_kms_helper
> > > syscopyarea
> > > > sysfillrect sysimgblt fb_sys_fops ttm igb ahci crc32c_intel ptp libahci
> > > > pps_core drm dca libata i2c_algo_bit i2c_core
> > > > [ 4556.061190] CPU: 54 PID: 9841 Comm: ibacm Tainted: G          I
> > > > 4.14.0-rc2+ #6
> > > > [ 4556.069667] Hardware name: Intel Corporation S2600WT2/S2600WT2,
> > > BIOS
> > > > SE5C610.86B.01.01.0008.021120151325 02/11/2015
> > > > [ 4556.081339] task: ffff880855f42d00 task.stack: ffffc900246b4000
> > > > [ 4556.087967] RIP: 0010:          (null)
> > > > [ 4556.092166] RSP: 0018:ffffc900246b7bc8 EFLAGS: 00010246
> > > > [ 4556.098018] RAX: ffffffff81dbe9e0 RBX: ffff881058bb1000 RCX:
> > > > 0000000000000000
> > > > [ 4556.105997] RDX: 0000000000001100 RSI: ffff881058bb1320 RDI:
> > > > ffff881056362000
> > > > [ 4556.113984] RBP: ffffc900246b7bf8 R08: 0000000000000ec0 R09:
> > > > 0000000000001100
> > > > [ 4556.121971] R10: ffff8810573a5000 R11: 0000000000000000 R12:
> > > > ffff881056362000
> > > > [ 4556.129957] R13: 0000000000000ec0 R14: ffff881058bb1320 R15:
> > > > 0000000000000ec0
> > > > [ 4556.137945] FS:  00007fe0ba5a38c0(0000) GS:ffff88105f080000(0000)
> > > > knlGS:0000000000000000
> > > > [ 4556.147000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > [ 4556.153433] CR2: 0000000000000000 CR3: 0000001056f5d003 CR4:
> > > > 00000000001606e0
> > > > [ 4556.161419] Call Trace:
> > > > [ 4556.164167]  ? netlink_dump+0x12c/0x290
> > > > [ 4556.168468]  __netlink_dump_start+0x186/0x1f0
> > > > [ 4556.173357]  rdma_nl_rcv_msg+0x193/0x1b0 [ib_core]
> > > > [ 4556.178724]  rdma_nl_rcv+0xdc/0x130 [ib_core]
> > > > [ 4556.183604]  netlink_unicast+0x181/0x240
> > > > [ 4556.187998]  netlink_sendmsg+0x2c2/0x3b0
> > > > [ 4556.192392]  sock_sendmsg+0x38/0x50
> > > > [ 4556.196299]  SYSC_sendto+0x102/0x190
> > > > [ 4556.200308]  ? __audit_syscall_entry+0xaf/0x100
> > > > [ 4556.205387]  ? syscall_trace_enter+0x1d0/0x2b0
> > > > [ 4556.210366]  ? __audit_syscall_exit+0x209/0x290
> > > > [ 4556.215442]  SyS_sendto+0xe/0x10
> > > > [ 4556.219060]  do_syscall_64+0x67/0x1b0
> > > > [ 4556.223165]  entry_SYSCALL64_slow_path+0x25/0x25
> > > > [ 4556.228328] RIP: 0033:0x7fe0b9db2a63
> > > > [ 4556.232333] RSP: 002b:00007ffc55edc260 EFLAGS: 00000293
> ORIG_RAX:
> > > > 000000000000002c
> > > > [ 4556.240808] RAX: ffffffffffffffda RBX: 0000000000000010 RCX:
> > > > 00007fe0b9db2a63
> > > > [ 4556.248796] RDX: 0000000000000010 RSI: 00007ffc55edc280 RDI:
> > > > 000000000000000d
> > > > [ 4556.256782] RBP: 00007ffc55edc670 R08: 00007ffc55edc270 R09:
> > > > 000000000000000c
> > > > [ 4556.265321] R10: 0000000000000000 R11: 0000000000000293 R12:
> > > > 00007ffc55edc280
> > > > [ 4556.273846] R13: 000000000260b400 R14: 000000000000000d R15:
> > > > 0000000000000001
> > > > [ 4556.282368] Code:  Bad RIP value.
> > > > [ 4556.286629] RIP:           (null) RSP: ffffc900246b7bc8
> > > > [ 4556.293013] CR2: 0000000000000000
> > > > [ 4556.297292] ---[ end trace 8d67abcfd10ec209 ]---
> > > > [ 4556.305465] Kernel panic - not syncing: Fatal exception
> > > > [ 4556.313786] Kernel Offset: disabled
> > > > [ 4556.321563] ---[ end Kernel panic - not syncing: Fatal exception
> > > > [ 4556.328960] ------------[ cut here ]------------
> > > >
> > > > Special case RDMA_NL_LS response messages to call the appropriate
> > > > callback.
> > > >
> > > > Additionally, make sure that the .dump() callback is not NULL
> > > > before calling it.
> > >
> > > Please don't add them here, it is dead code in wrong place.
> > > First it is unachievable paths, and second they need to be checked
> > > in is_nl_valid() function.
> >
> > The check in is_nl_valid() is for .doit OR .done.  There is no context based on
> the index.
> > So if a specific index has one or the other is_nl_valid is true.
> >
> > This is what caused the Oops (index only had .doit callbacks, but .dump path
> > was chosen).  So I don't think that these are unachievable paths.
> >
> > The is_nl_valid() function is incomplete.  This shows that packets can be
> crafted
> > to cause the wrong path to be taken, and if there is no callback function we
> will
> > have the same issue.  Specific index validation is needed, and should probably
> be
> > addressed with a different patch.
> 
> .doit exists for RDMA_NL_LS and for other .dumpit exists, so no actual
> check is needed.
> 
> if you want to be on the safe side, please extend is_nl_valid(). IMHO,
> it will keep code cleaner, the checks in one localized place and execution
> is not mixed with the validity checks.

Leon,

I do not disagree with you on this.  My patch is to fix the Oops, and make
sure it can't happen with the current code.

Extending is_nl_valid() to be more complete seems to be out of scope for
the current issue of making the Oops go away.

I can probably work on is_nl_valid(), but I will need much more time to make sure
that it is correct, and don't feel that I can get it done with in the appropriate
time frame (i.e. sometime this week).

Mike

 
> Thanks
> 
> >
> > Thanks,
> >
> > M
> > > Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Doug Ledford]

On Tue, 2017-10-24 at 08:41 -0400, Michael J. Ruhl wrote:
> From: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> rdma_nl_rcv_msg() checks to see if it should use the .dump() callback
> or the .doit() callback.  The check is done with this check:
> 
> if (flags & NLM_F_DUMP) ...
> 
> The NLM_F_DUMP flag is two bits (NLM_F_ROOT | NLM_F_MATCH).
> 
> When an RDMA_NL_LS message (response) is received, the bit used for
> indicating an error is the same bit as NLM_F_ROOT.
> 
> NLM_F_ROOT == (0x100) == RDMA_NL_LS_F_ERR.

What are the remaining flags in the failing error case?

Or to be more specific,

> 
>  	/* FIXME: Convert IWCM to properly handle doit callbacks */
>  	if ((nlh->nlmsg_flags & NLM_F_DUMP)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This test is technically faulty.  Since NLM_F_DUMP is a multi-bit flag,
it must be ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) to be
technically correct.  So, my question then becomes, if we correct this
test, will the RDMA_NL_LS_F_ERR return message still trigger this wrong
path?  I'd rather have a technically correct fix to this if statement
than a special case of the index value if possible.



-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Doug Ledford]

On Tue, 2017-10-24 at 15:42 +0000, Ruhl, Michael J wrote:
> > > have the same issue.  Specific index validation is needed, and
> > > should probably
> > 
> > be
> > > addressed with a different patch.
> > 
> > .doit exists for RDMA_NL_LS and for other .dumpit exists, so no
> > actual
> > check is needed.
> > 
> > if you want to be on the safe side, please extend is_nl_valid().
> > IMHO,
> > it will keep code cleaner, the checks in one localized place and
> > execution
> > is not mixed with the validity checks.
> 
> Leon,
> 
> I do not disagree with you on this.  My patch is to fix the Oops, and
> make
> sure it can't happen with the current code.
> 
> Extending is_nl_valid() to be more complete seems to be out of scope
> for
> the current issue of making the Oops go away.
> 
> I can probably work on is_nl_valid(), but I will need much more time
> to make sure
> that it is correct, and don't feel that I can get it done with in the
> appropriate
> time frame (i.e. sometime this week).

I agree with Michael that tweaking this code to be optimal versus
plainly failsafe is a patch for another day.  I've taken this one. 
Thanks Michael.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On Wed, Oct 25, 2017 at 02:57:20PM -0400, Doug Ledford wrote:
> On Tue, 2017-10-24 at 15:42 +0000, Ruhl, Michael J wrote:
> > > > have the same issue.  Specific index validation is needed, and
> > > > should probably
> > >
> > > be
> > > > addressed with a different patch.
> > >
> > > .doit exists for RDMA_NL_LS and for other .dumpit exists, so no
> > > actual
> > > check is needed.
> > >
> > > if you want to be on the safe side, please extend is_nl_valid().
> > > IMHO,
> > > it will keep code cleaner, the checks in one localized place and
> > > execution
> > > is not mixed with the validity checks.
> >
> > Leon,
> >
> > I do not disagree with you on this.  My patch is to fix the Oops, and
> > make
> > sure it can't happen with the current code.
> >
> > Extending is_nl_valid() to be more complete seems to be out of scope
> > for
> > the current issue of making the Oops go away.
> >
> > I can probably work on is_nl_valid(), but I will need much more time
> > to make sure
> > that it is correct, and don't feel that I can get it done with in the
> > appropriate
> > time frame (i.e. sometime this week).
>
> I agree with Michael that tweaking this code to be optimal versus
> plainly failsafe is a patch for another day.  I've taken this one.
> Thanks Michael.

Too bad, you actually suggested very good solution, it is unclear who
will now implement it.

Thanks

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Doug Ledford]

On Wed, 2017-10-25 at 22:06 +0300, Leon Romanovsky wrote:
> > I agree with Michael that tweaking this code to be optimal versus
> > plainly failsafe is a patch for another day.  I've taken this one.
> > Thanks Michael.
> 
> Too bad, you actually suggested very good solution, it is unclear who
> will now implement it.

I may do it myself.  I've been looking at the code and I don't like the
if construct because I don't feel it conveys the purpose of the code
clearly.  Implicit in the if construct is embedded knowledge about
which providers do what.  I'd rather have a switch construct on the
index with the flow split to those that have only .dump, those that
have only .doit, and those that have both.  The current if doesn't make
it clear that this is how the netlink services are structured.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1417 bytes --]

On Wed, Oct 25, 2017 at 03:17:28PM -0400, Doug Ledford wrote:
> On Wed, 2017-10-25 at 22:06 +0300, Leon Romanovsky wrote:
> > > I agree with Michael that tweaking this code to be optimal versus
> > > plainly failsafe is a patch for another day.  I've taken this one.
> > > Thanks Michael.
> >
> > Too bad, you actually suggested very good solution, it is unclear who
> > will now implement it.
>
> I may do it myself.  I've been looking at the code and I don't like the
> if construct because I don't feel it conveys the purpose of the code
> clearly.  Implicit in the if construct is embedded knowledge about
> which providers do what.  I'd rather have a switch construct on the
> index with the flow split to those that have only .dump, those that
> have only .doit, and those that have both.  The current if doesn't make
> it clear that this is how the netlink services are structured.

Most probably you are right, I just need to see the code to be sure.

The original "if" was outcome of my wrong expectation that someone will
help me to modernize IWCM. Such modernization was supposed to remove
nasty "|| index .. " checks.

So my final goal is to see the following code:
if(dump_flag)
	return do_dump

return do_doit

Thanks

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]