From 1d49eb91e86e8c1c1614c72e3e958b6b7e2472a9 Mon Sep 17 00:00:00 2001 From: Ioanna Alifieraki Date: Mon, 15 Nov 2021 15:16:45 +0200 Subject: [PATCH 1/5] ipmi: Move remove_work to dedicated workqueue Currently when removing an ipmi_user the removal is deferred as a work on the system's workqueue. Although this guarantees the free operation will occur in non atomic context, it can race with the ipmi_msghandler module removal (see [1]) . In case a remove_user work is scheduled for removal and shortly after ipmi_msghandler module is removed we can end up in a situation where the module is removed fist and when the work is executed the system crashes with : BUG: unable to handle page fault for address: ffffffffc05c3450 PF: supervisor instruction fetch in kernel mode PF: error_code(0x0010) - not-present page because the pages of the module are gone. In cleanup_ipmi() there is no easy way to detect if there are any pending works to flush them before removing the module. This patch creates a separate workqueue and schedules the remove_work works on it. When removing the module the workqueue is drained when destroyed to avoid the race. [1] https://bugs.launchpad.net/bugs/1950666 Cc: stable@vger.kernel.org # 5.1 Fixes: 3b9a907223d7 (ipmi: fix sleep-in-atomic in free_user at cleanup SRCU user->release_barrier) Signed-off-by: Ioanna Alifieraki Message-Id: <20211115131645.25116-1-ioanna-maria.alifieraki@canonical.com> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index deed355422f4..1ade72bfae0f 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -191,6 +191,8 @@ struct ipmi_user { struct work_struct remove_work; }; +struct workqueue_struct *remove_work_wq; + static struct ipmi_user *acquire_ipmi_user(struct ipmi_user *user, int *index) __acquires(user->release_barrier) { @@ -1297,7 +1299,7 @@ static void free_user(struct kref *ref) struct ipmi_user *user = container_of(ref, struct ipmi_user, refcount); /* SRCU cleanup must happen in task context. */ - schedule_work(&user->remove_work); + queue_work(remove_work_wq, &user->remove_work); } static void _ipmi_destroy_user(struct ipmi_user *user) @@ -5383,6 +5385,13 @@ static int ipmi_init_msghandler(void) atomic_notifier_chain_register(&panic_notifier_list, &panic_block); + remove_work_wq = create_singlethread_workqueue("ipmi-msghandler-remove-wq"); + if (!remove_work_wq) { + pr_err("unable to create ipmi-msghandler-remove-wq workqueue"); + rv = -ENOMEM; + goto out; + } + initialized = true; out: @@ -5408,6 +5417,8 @@ static void __exit cleanup_ipmi(void) int count; if (initialized) { + destroy_workqueue(remove_work_wq); + atomic_notifier_chain_unregister(&panic_notifier_list, &panic_block); From 5a3ba99b62d8486de0316334e72ac620d4b94fdd Mon Sep 17 00:00:00 2001 From: Wei Yongjun Date: Tue, 23 Nov 2021 08:36:18 +0000 Subject: [PATCH 2/5] ipmi: msghandler: Make symbol 'remove_work_wq' static The sparse tool complains as follows: drivers/char/ipmi/ipmi_msghandler.c:194:25: warning: symbol 'remove_work_wq' was not declared. Should it be static? This symbol is not used outside of ipmi_msghandler.c, so marks it static. Fixes: 1d49eb91e86e ("ipmi: Move remove_work to dedicated workqueue") Reported-by: Hulk Robot Signed-off-by: Wei Yongjun Message-Id: <20211123083618.2366808-1-weiyongjun1@huawei.com> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 1ade72bfae0f..a2ec0171363a 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -191,7 +191,7 @@ struct ipmi_user { struct work_struct remove_work; }; -struct workqueue_struct *remove_work_wq; +static struct workqueue_struct *remove_work_wq; static struct ipmi_user *acquire_ipmi_user(struct ipmi_user *user, int *index) __acquires(user->release_barrier) From c33fdfbabb6c930454df017f3cd3507dc1a87d09 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Wed, 24 Nov 2021 13:03:23 -0800 Subject: [PATCH 3/5] ipmi: fix oob access due to uninit smi_msg type We're hitting OOB accesses in handle_ipmb_direct_rcv_rsp() (memcpy of size -1) after user space generates a message. Looks like the message is incorrectly assumed to be of the new IPMB type, because type is never set and message is allocated with kmalloc() not kzalloc(). Fixes: 059747c245f0 ("ipmi: Add support for IPMB direct messages") Signed-off-by: Jakub Kicinski Message-Id: <20211124210323.1950976-1-kuba@kernel.org> Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index a2ec0171363a..7d7df17d8b3d 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -5033,6 +5033,7 @@ struct ipmi_smi_msg *ipmi_alloc_smi_msg(void) if (rv) { rv->done = free_smi_msg; rv->user_data = NULL; + rv->type = IPMI_SMI_MSG_TYPE_NORMAL; atomic_inc(&smi_msg_inuse_count); } return rv; From d2c12f56fa97df216e71437b218ffbeeb4dd46aa Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Thu, 25 Nov 2021 08:47:27 -0600 Subject: [PATCH 4/5] ipmi: fix IPMI_SMI_MSG_TYPE_IPMB_DIRECT response length checking A couple of issues: The tested data sizes are wrong; during the design that changed and this got missed. The formatting of the reponse couldn't use the normal one, it has to be an IPMB formatted response. Reported-by: Jakub Kicinski Fixes: 059747c245f0 ("ipmi: Add support for IPMB direct messages") Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 7d7df17d8b3d..99ea6d9b3716 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -4457,13 +4457,24 @@ return_unspecified: msg->rsp[2] = IPMI_ERR_UNSPECIFIED; msg->rsp_size = 3; } else if (msg->type == IPMI_SMI_MSG_TYPE_IPMB_DIRECT) { - /* commands must have at least 3 bytes, responses 4. */ - if (is_cmd && (msg->rsp_size < 3)) { + /* commands must have at least 4 bytes, responses 5. */ + if (is_cmd && (msg->rsp_size < 4)) { ipmi_inc_stat(intf, invalid_commands); goto out; } - if (!is_cmd && (msg->rsp_size < 4)) - goto return_unspecified; + if (!is_cmd && (msg->rsp_size < 5)) { + ipmi_inc_stat(intf, invalid_ipmb_responses); + /* Construct a valid error response. */ + msg->rsp[0] = msg->data[0] & 0xfc; /* NetFN */ + msg->rsp[0] |= (1 << 2); /* Make it a response */ + msg->rsp[0] |= msg->data[2] & 3; /* rqLUN */ + msg->rsp[1] = msg->data[1]; /* Addr */ + msg->rsp[2] = msg->data[2] & 0xfc; /* rqSeq */ + msg->rsp[2] |= msg->data[0] & 0x3; /* rsLUN */ + msg->rsp[3] = msg->data[3]; /* Cmd */ + msg->rsp[4] = IPMI_ERR_UNSPECIFIED; + msg->rsp_size = 5; + } } else if ((msg->data_size >= 2) && (msg->data[0] == (IPMI_NETFN_APP_REQUEST << 2)) && (msg->data[1] == IPMI_SEND_MSG_CMD) From c03a487a83fddbca1ef6cb5b97a69cd3e390e233 Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Thu, 25 Nov 2021 11:23:20 -0600 Subject: [PATCH 5/5] ipmi:ipmb: Fix unknown command response More missed changes, the response back to another system sending a command that had no user to handle it wasn't formatted properly. Signed-off-by: Corey Minyard --- drivers/char/ipmi/ipmi_msghandler.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 99ea6d9b3716..c837d5416e0e 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -3920,9 +3920,11 @@ static int handle_ipmb_direct_rcv_cmd(struct ipmi_smi *intf, /* We didn't find a user, deliver an error response. */ ipmi_inc_stat(intf, unhandled_commands); - msg->data[0] = ((netfn + 1) << 2) | (msg->rsp[4] & 0x3); - msg->data[1] = msg->rsp[2]; - msg->data[2] = msg->rsp[4] & ~0x3; + msg->data[0] = (netfn + 1) << 2; + msg->data[0] |= msg->rsp[2] & 0x3; /* rqLUN */ + msg->data[1] = msg->rsp[1]; /* Addr */ + msg->data[2] = msg->rsp[2] & ~0x3; /* rqSeq */ + msg->data[2] |= msg->rsp[0] & 0x3; /* rsLUN */ msg->data[3] = cmd; msg->data[4] = IPMI_INVALID_CMD_COMPLETION_CODE; msg->data_size = 5;