| Message ID | 20161121114417.GA4158@leon.nu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Nov 21, 2016 at 12:44 PM, Leon Romanovsky <leon@kernel.org> wrote: > On Mon, Nov 21, 2016 at 11:30:21AM +0100, Dmitry Vyukov wrote: >> On Mon, Nov 21, 2016 at 11:25 AM, Miroslav Benes <mbenes@suse.cz> wrote: >> > On Mon, 21 Nov 2016, Dmitry Vyukov wrote: >> > >> >> WARNINGs mean kernel bugs. >> >> The one in ucma_write() points to user programming error >> >> or a malicious attempt. This is not a kernel bug, remove it. >> >> >> >> BUG/WARNs that are not kernel bugs hinder automated testing effots. >> >> >> >> Signed-off-by: Dmitry Vyukov <dvyukov@google.com> >> >> Cc: Doug Ledford <dledford@redhat.com> >> >> Cc: Sean Hefty <sean.hefty@intel.com> >> >> Cc: Hal Rosenstock <hal.rosenstock@gmail.com> >> >> Cc: Leon Romanovsky <leon@kernel.org> >> >> Cc: linux-rdma@vger.kernel.org >> >> Cc: linux-kernel@vger.kernel.org >> >> Cc: syzkaller@googlegroups.com >> >> >> >> --- >> >> Changes since v1: >> >> - added printk_once >> >> --- >> >> drivers/infiniband/core/ucma.c | 5 ++++- >> >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c >> >> index 9520154..405d0ce 100644 >> >> --- a/drivers/infiniband/core/ucma.c >> >> +++ b/drivers/infiniband/core/ucma.c >> >> @@ -1584,8 +1584,11 @@ static ssize_t ucma_write(struct file *filp, const char __user *buf, >> >> struct rdma_ucm_cmd_hdr hdr; >> >> ssize_t ret; >> >> >> >> - if (WARN_ON_ONCE(!ib_safe_file_access(filp))) >> >> + if (!ib_safe_file_access(filp)) { >> >> + printk_once("ucma_write: process %d (%s) tried to do something hinky\n", >> >> + task_tgid_vnr(current), current->comm); >> >> return -EACCES; >> >> + } >> >> >> >> if (len < sizeof(hdr)) >> >> return -EINVAL; >> > >> > FWIW, WARN_ON_ONCE came with commit e6bd18f57aad ("IB/security: Restrict >> > use of the write() interface"). Would it make sense to change the other >> > places as well? >> >> >> I guess so. >> Can I ask somebody of infiniband maintainers to take care of this? > > Please see below, > Hope it helps. In ib_ucm_write function there is a wrong prefix: + pr_err_once("ucm_write: process %d (%s) tried to do something hinky\n", Otherwise looks good. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 21, 2016 at 12:48:35PM +0100, Dmitry Vyukov wrote: > On Mon, Nov 21, 2016 at 12:44 PM, Leon Romanovsky <leon@kernel.org> wrote: > > On Mon, Nov 21, 2016 at 11:30:21AM +0100, Dmitry Vyukov wrote: > >> On Mon, Nov 21, 2016 at 11:25 AM, Miroslav Benes <mbenes@suse.cz> wrote: > >> > On Mon, 21 Nov 2016, Dmitry Vyukov wrote: > >> > > >> >> WARNINGs mean kernel bugs. > >> >> The one in ucma_write() points to user programming error > >> >> or a malicious attempt. This is not a kernel bug, remove it. > >> >> > >> >> BUG/WARNs that are not kernel bugs hinder automated testing effots. > >> >> > >> >> Signed-off-by: Dmitry Vyukov <dvyukov@google.com> > >> >> Cc: Doug Ledford <dledford@redhat.com> > >> >> Cc: Sean Hefty <sean.hefty@intel.com> > >> >> Cc: Hal Rosenstock <hal.rosenstock@gmail.com> > >> >> Cc: Leon Romanovsky <leon@kernel.org> > >> >> Cc: linux-rdma@vger.kernel.org > >> >> Cc: linux-kernel@vger.kernel.org > >> >> Cc: syzkaller@googlegroups.com > >> >> > >> >> --- > >> >> Changes since v1: > >> >> - added printk_once > >> >> --- > >> >> drivers/infiniband/core/ucma.c | 5 ++++- > >> >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> >> > >> >> diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > >> >> index 9520154..405d0ce 100644 > >> >> --- a/drivers/infiniband/core/ucma.c > >> >> +++ b/drivers/infiniband/core/ucma.c > >> >> @@ -1584,8 +1584,11 @@ static ssize_t ucma_write(struct file *filp, const char __user *buf, > >> >> struct rdma_ucm_cmd_hdr hdr; > >> >> ssize_t ret; > >> >> > >> >> - if (WARN_ON_ONCE(!ib_safe_file_access(filp))) > >> >> + if (!ib_safe_file_access(filp)) { > >> >> + printk_once("ucma_write: process %d (%s) tried to do something hinky\n", > >> >> + task_tgid_vnr(current), current->comm); > >> >> return -EACCES; > >> >> + } > >> >> > >> >> if (len < sizeof(hdr)) > >> >> return -EINVAL; > >> > > >> > FWIW, WARN_ON_ONCE came with commit e6bd18f57aad ("IB/security: Restrict > >> > use of the write() interface"). Would it make sense to change the other > >> > places as well? > >> > >> > >> I guess so. > >> Can I ask somebody of infiniband maintainers to take care of this? > > > > Please see below, > > Hope it helps. > > In ib_ucm_write function there is a wrong prefix: > > + pr_err_once("ucm_write: process %d (%s) tried to do something hinky\n", I did it intentionally to have the same errors for all flows. > > Otherwise looks good. Thanks.
On Mon, Nov 21, 2016 at 02:14:08PM +0200, Leon Romanovsky wrote: > > > > In ib_ucm_write function there is a wrong prefix: > > > > + pr_err_once("ucm_write: process %d (%s) tried to do something hinky\n", > > I did it intentionally to have the same errors for all flows. Lets actually use a good message too please? pr_err_once("ucm_write: process %d (%s) changed security contexts after opening FD, this is not allowed.\n", Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From e0bfe4771591f9ffbcaa4e66d6257ed95e2d7188 Mon Sep 17 00:00:00 2001
From: Leon Romanovsky <leonro@mellanox.com>
Date: Mon, 21 Nov 2016 13:30:59 +0200
Subject: [PATCH] IB/{core, qib}: Remove WARN that is not kernel bug
WARNINGs mean kernel bugs, in this case, they are placed
to mark programming errors and/or malicious attempts.
BUG/WARNs that are not kernel bugs hinder automated testing efforts.
Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/ucm.c | 5 ++++-
drivers/infiniband/core/ucma.c | 5 ++++-
drivers/infiniband/core/uverbs_main.c | 5 ++++-
drivers/infiniband/hw/qib/qib_file_ops.c | 5 ++++-
4 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
index 7713ef0..0076b55 100644
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -1104,8 +1104,11 @@ static ssize_t ib_ucm_write(struct file *filp, const char __user *buf,
struct ib_ucm_cmd_hdr hdr;
ssize_t result;
- if (WARN_ON_ONCE(!ib_safe_file_access(filp)))
+ if (!ib_safe_file_access(filp)) {
+ pr_err_once("ucm_write: process %d (%s) tried to do something hinky\n",
+ task_tgid_vnr(current), current->comm);
return -EACCES;
+ }
if (len < sizeof(hdr))
return -EINVAL;
diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
index 9520154..31fa27f 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -1584,8 +1584,11 @@ static ssize_t ucma_write(struct file *filp, const char __user *buf,
struct rdma_ucm_cmd_hdr hdr;
ssize_t ret;
- if (WARN_ON_ONCE(!ib_safe_file_access(filp)))
+ if (!ib_safe_file_access(filp)) {
+ pr_err_once("ucma_write: process %d (%s) tried to do something hinky\n",
+ task_tgid_vnr(current), current->comm);
return -EACCES;
+ }
if (len < sizeof(hdr))
return -EINVAL;
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 44b1104..329eb15 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -746,8 +746,11 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
int srcu_key;
ssize_t ret;
- if (WARN_ON_ONCE(!ib_safe_file_access(filp)))
+ if (!ib_safe_file_access(filp)) {
+ pr_err_once("uverbs_write: process %d (%s) tried to do something hinky\n",
+ task_tgid_vnr(current), current->comm);
return -EACCES;
+ }
if (count < sizeof hdr)
return -EINVAL;
diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
index 382466a..b43ea52 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -2066,8 +2066,11 @@ static ssize_t qib_write(struct file *fp, const char __user *data,
ssize_t ret = 0;
void *dest;
- if (WARN_ON_ONCE(!ib_safe_file_access(fp)))
+ if (!ib_safe_file_access(fp)) {
+ pr_err_once("qib_write: process %d (%s) tried to do something hinky\n",
+ task_tgid_vnr(current), current->comm);
return -EACCES;
+ }
if (count < sizeof(cmd.type)) {
ret = -EINVAL;
--
2.7.4