From: Bhaumik Bhatt bbhatt@codeaurora.org
MHI reads the channel ID from the event ring element sent by the device which can be any value between 0 and 255. In order to prevent any out of bound accesses, add a check against the maximum number of channels supported by the controller and those channels not configured yet so as to skip processing of that event ring element.
Cc: stable@vger.kernel.org Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device") Signed-off-by: Bhaumik Bhatt bbhatt@codeaurora.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Reviewed-by: Jeffrey Hugo quic_jhugo@quicinc.com Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora... Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org --- drivers/bus/mhi/core/main.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index 22acde118bc3..ed07421c4870 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, cmd_pkt = mhi_to_virtual(mhi_ring, ptr);
chan = MHI_TRE_GET_CMD_CHID(cmd_pkt); - mhi_chan = &mhi_cntrl->mhi_chan[chan]; - write_lock_bh(&mhi_chan->lock); - mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre); - complete(&mhi_chan->completion); - write_unlock_bh(&mhi_chan->lock); + WARN_ON(chan >= mhi_cntrl->max_chan); + + if (chan < mhi_cntrl->max_chan && + mhi_cntrl->mhi_chan[chan].configured) { + mhi_chan = &mhi_cntrl->mhi_chan[chan]; + write_lock_bh(&mhi_chan->lock); + mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre); + complete(&mhi_chan->completion); + write_unlock_bh(&mhi_chan->lock); + }
mhi_del_ring_element(mhi_cntrl, mhi_ring); }
On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
From: Bhaumik Bhatt bbhatt@codeaurora.org
MHI reads the channel ID from the event ring element sent by the device which can be any value between 0 and 255. In order to prevent any out of bound accesses, add a check against the maximum number of channels supported by the controller and those channels not configured yet so as to skip processing of that event ring element.
Cc: stable@vger.kernel.org Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device") Signed-off-by: Bhaumik Bhatt bbhatt@codeaurora.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Reviewed-by: Jeffrey Hugo quic_jhugo@quicinc.com Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora... Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
drivers/bus/mhi/core/main.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index 22acde118bc3..ed07421c4870 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, cmd_pkt = mhi_to_virtual(mhi_ring, ptr); chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
- mhi_chan = &mhi_cntrl->mhi_chan[chan];
- write_lock_bh(&mhi_chan->lock);
- mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
- complete(&mhi_chan->completion);
- write_unlock_bh(&mhi_chan->lock);
- WARN_ON(chan >= mhi_cntrl->max_chan);
What can ever trigger this WARN_ON()? Do you mean to reboot a machine if panic-on-warn is set?
If this can actually happen, then check for it and recover properly, don't just blindly warn and then keep on going.
thanks,
greg k-h
On Thu, Jun 24, 2021 at 03:50:45PM +0200, Greg KH wrote:
On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
From: Bhaumik Bhatt bbhatt@codeaurora.org
MHI reads the channel ID from the event ring element sent by the device which can be any value between 0 and 255. In order to prevent any out of bound accesses, add a check against the maximum number of channels supported by the controller and those channels not configured yet so as to skip processing of that event ring element.
Cc: stable@vger.kernel.org Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device") Signed-off-by: Bhaumik Bhatt bbhatt@codeaurora.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Reviewed-by: Jeffrey Hugo quic_jhugo@quicinc.com Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora... Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
drivers/bus/mhi/core/main.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index 22acde118bc3..ed07421c4870 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, cmd_pkt = mhi_to_virtual(mhi_ring, ptr); chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
- mhi_chan = &mhi_cntrl->mhi_chan[chan];
- write_lock_bh(&mhi_chan->lock);
- mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
- complete(&mhi_chan->completion);
- write_unlock_bh(&mhi_chan->lock);
- WARN_ON(chan >= mhi_cntrl->max_chan);
What can ever trigger this WARN_ON()? Do you mean to reboot a machine if panic-on-warn is set?
If this can actually happen, then check for it and recover properly, don't just blindly warn and then keep on going.
We can't do much here other than warning the user and dropping the command.
There is no recovery possible because, the device has sent the command completion event on a wrong channel. It can't happen usually unless a malcious device sits on the other end.
Thanks, Mani
thanks,
greg k-h
On Thu, Jun 24, 2021 at 08:02:48PM +0530, Manivannan Sadhasivam wrote:
On Thu, Jun 24, 2021 at 03:50:45PM +0200, Greg KH wrote:
On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
From: Bhaumik Bhatt bbhatt@codeaurora.org
MHI reads the channel ID from the event ring element sent by the device which can be any value between 0 and 255. In order to prevent any out of bound accesses, add a check against the maximum number of channels supported by the controller and those channels not configured yet so as to skip processing of that event ring element.
Cc: stable@vger.kernel.org Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device") Signed-off-by: Bhaumik Bhatt bbhatt@codeaurora.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Reviewed-by: Jeffrey Hugo quic_jhugo@quicinc.com Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora... Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
drivers/bus/mhi/core/main.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index 22acde118bc3..ed07421c4870 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, cmd_pkt = mhi_to_virtual(mhi_ring, ptr); chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
- mhi_chan = &mhi_cntrl->mhi_chan[chan];
- write_lock_bh(&mhi_chan->lock);
- mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
- complete(&mhi_chan->completion);
- write_unlock_bh(&mhi_chan->lock);
- WARN_ON(chan >= mhi_cntrl->max_chan);
What can ever trigger this WARN_ON()? Do you mean to reboot a machine if panic-on-warn is set?
If this can actually happen, then check for it and recover properly, don't just blindly warn and then keep on going.
We can't do much here other than warning the user and dropping the command.
But you didn't warn anyone. Well, you rebooted the machine, is that ok? If this can be triggered by a user, this should never happen.
Do not use WARN_ON() ever please.
There is no recovery possible because, the device has sent the command completion event on a wrong channel. It can't happen usually unless a malcious device sits on the other end.
Then just eat the message and move on, please do not crash the box.
thanks,
gre k-h
On Thu, Jun 24, 2021 at 04:39:51PM +0200, Greg KH wrote:
On Thu, Jun 24, 2021 at 08:02:48PM +0530, Manivannan Sadhasivam wrote:
On Thu, Jun 24, 2021 at 03:50:45PM +0200, Greg KH wrote:
On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
From: Bhaumik Bhatt bbhatt@codeaurora.org
MHI reads the channel ID from the event ring element sent by the device which can be any value between 0 and 255. In order to prevent any out of bound accesses, add a check against the maximum number of channels supported by the controller and those channels not configured yet so as to skip processing of that event ring element.
Cc: stable@vger.kernel.org Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device") Signed-off-by: Bhaumik Bhatt bbhatt@codeaurora.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Reviewed-by: Jeffrey Hugo quic_jhugo@quicinc.com Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora... Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
drivers/bus/mhi/core/main.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index 22acde118bc3..ed07421c4870 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, cmd_pkt = mhi_to_virtual(mhi_ring, ptr); chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
- mhi_chan = &mhi_cntrl->mhi_chan[chan];
- write_lock_bh(&mhi_chan->lock);
- mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
- complete(&mhi_chan->completion);
- write_unlock_bh(&mhi_chan->lock);
- WARN_ON(chan >= mhi_cntrl->max_chan);
What can ever trigger this WARN_ON()? Do you mean to reboot a machine if panic-on-warn is set?
If this can actually happen, then check for it and recover properly, don't just blindly warn and then keep on going.
We can't do much here other than warning the user and dropping the command.
But you didn't warn anyone. Well, you rebooted the machine, is that ok? If this can be triggered by a user, this should never happen.
Do not use WARN_ON() ever please.
There is no recovery possible because, the device has sent the command completion event on a wrong channel. It can't happen usually unless a malcious device sits on the other end.
Then just eat the message and move on, please do not crash the box.
Okay. We'll spit an error message and drop the event.
Thanks, Mani
thanks,
gre k-h
On Thu, Jun 24, 2021 at 08:17:52PM +0530, Manivannan Sadhasivam wrote:
On Thu, Jun 24, 2021 at 04:39:51PM +0200, Greg KH wrote:
On Thu, Jun 24, 2021 at 08:02:48PM +0530, Manivannan Sadhasivam wrote:
On Thu, Jun 24, 2021 at 03:50:45PM +0200, Greg KH wrote:
On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
From: Bhaumik Bhatt bbhatt@codeaurora.org
MHI reads the channel ID from the event ring element sent by the device which can be any value between 0 and 255. In order to prevent any out of bound accesses, add a check against the maximum number of channels supported by the controller and those channels not configured yet so as to skip processing of that event ring element.
Cc: stable@vger.kernel.org Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device") Signed-off-by: Bhaumik Bhatt bbhatt@codeaurora.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Reviewed-by: Jeffrey Hugo quic_jhugo@quicinc.com Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora... Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
drivers/bus/mhi/core/main.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index 22acde118bc3..ed07421c4870 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, cmd_pkt = mhi_to_virtual(mhi_ring, ptr); chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
- mhi_chan = &mhi_cntrl->mhi_chan[chan];
- write_lock_bh(&mhi_chan->lock);
- mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
- complete(&mhi_chan->completion);
- write_unlock_bh(&mhi_chan->lock);
- WARN_ON(chan >= mhi_cntrl->max_chan);
What can ever trigger this WARN_ON()? Do you mean to reboot a machine if panic-on-warn is set?
If this can actually happen, then check for it and recover properly, don't just blindly warn and then keep on going.
We can't do much here other than warning the user and dropping the command.
But you didn't warn anyone. Well, you rebooted the machine, is that ok? If this can be triggered by a user, this should never happen.
Do not use WARN_ON() ever please.
There is no recovery possible because, the device has sent the command completion event on a wrong channel. It can't happen usually unless a malcious device sits on the other end.
Then just eat the message and move on, please do not crash the box.
Okay. We'll spit an error message and drop the event.
If this can be triggered by a user, don't provide a way to DoS the kernel error log.
thanks,
greg k-h
On Thu, Jun 24, 2021 at 05:27:17PM +0200, Greg KH wrote:
On Thu, Jun 24, 2021 at 08:17:52PM +0530, Manivannan Sadhasivam wrote:
On Thu, Jun 24, 2021 at 04:39:51PM +0200, Greg KH wrote:
On Thu, Jun 24, 2021 at 08:02:48PM +0530, Manivannan Sadhasivam wrote:
On Thu, Jun 24, 2021 at 03:50:45PM +0200, Greg KH wrote:
On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
From: Bhaumik Bhatt bbhatt@codeaurora.org
MHI reads the channel ID from the event ring element sent by the device which can be any value between 0 and 255. In order to prevent any out of bound accesses, add a check against the maximum number of channels supported by the controller and those channels not configured yet so as to skip processing of that event ring element.
Cc: stable@vger.kernel.org Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device") Signed-off-by: Bhaumik Bhatt bbhatt@codeaurora.org Reviewed-by: Hemant Kumar hemantk@codeaurora.org Reviewed-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org Reviewed-by: Jeffrey Hugo quic_jhugo@quicinc.com Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora... Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
drivers/bus/mhi/core/main.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index 22acde118bc3..ed07421c4870 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, cmd_pkt = mhi_to_virtual(mhi_ring, ptr); chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
- mhi_chan = &mhi_cntrl->mhi_chan[chan];
- write_lock_bh(&mhi_chan->lock);
- mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
- complete(&mhi_chan->completion);
- write_unlock_bh(&mhi_chan->lock);
- WARN_ON(chan >= mhi_cntrl->max_chan);
What can ever trigger this WARN_ON()? Do you mean to reboot a machine if panic-on-warn is set?
If this can actually happen, then check for it and recover properly, don't just blindly warn and then keep on going.
We can't do much here other than warning the user and dropping the command.
But you didn't warn anyone. Well, you rebooted the machine, is that ok? If this can be triggered by a user, this should never happen.
Do not use WARN_ON() ever please.
There is no recovery possible because, the device has sent the command completion event on a wrong channel. It can't happen usually unless a malcious device sits on the other end.
Then just eat the message and move on, please do not crash the box.
Okay. We'll spit an error message and drop the event.
If this can be triggered by a user, don't provide a way to DoS the kernel error log.
The term "user" is a bit vague here. Only a malcious device sits on the PCIe bus that claims a defined VID/PID can trigger this error. And we do need to tell the user of the host machine that the device tried to do something wrong.
So I guess the error log is fine here?
Thanks, Mani
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Manivannan Sadhasivam