linux-kernel - Re: [PATCH] mailbox: Fix undefined behavior in of_mbox_index

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250917150135.GA28673@nxa18884-linux.ap.freescale.net>
Date: Wed, 17 Sep 2025 23:01:35 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Joonwon Kang <joonwonkang@...gle.com>
Cc: jassisinghbrar@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mailbox: Fix undefined behavior in of_mbox_index_xlate()

On Tue, Sep 16, 2025 at 03:34:55PM +0000, Joonwon Kang wrote:
>Although it is guided that `#mbox-cells` must be at least 1, there are
>many instances of `#mbox-cells = <0>;` in the device tree. If that is
>the case and the corresponding mailbox controller does not provide
>`of_xlate` function pointer, `of_mbox_index_xlate()` will be used by
>default and undefined behaviors could occur in that function.
>
>Below is a problematic control flow which results in undefined behavior
>when `#mbox-cells = <0>;`.
>
>```
>static struct mbox_chan *
>of_mbox_index_xlate(struct mbox_controller *mbox,
>                    const struct of_phandle_args *sp)
>{
>    int ind = sp->args[0];					// (4)
>
>    if (ind >= mbox->num_chans)					// (5)
>        return ERR_PTR(-EINVAL);
>
>    return &mbox->chans[ind];					// (6)
>}
>
>struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
>{
>    struct of_phandle_args spec;				// (1)
>
>    if (of_parse_phandle_with_args(dev->of_node, "mboxes",	// (2)
>        "#mbox-cells", index, &spec)) {
>        ...
>    }
>
>    list_for_each_entry(mbox, &mbox_cons, node)
>        if (mbox->dev->of_node == spec.np) {
>            chan = mbox->of_xlate(mbox, &spec);			// (3)
>            if (!IS_ERR(chan))
>                break;
>        }
>    ...
>    ret = __mbox_bind_client(chan, cl);				// (7)
>    ...
>}
>
>static int __mbox_bind_client(struct mbox_chan *chan,
>                              struct mbox_client *cl)
>{
>    if (chan->cl || ...) {					// (8)
>}
>````
>
>(1) `spec.args[]` is filled with arbitrary leftover values in the stack.
>    Let's say that `spec.args[0] == 0xffffffff`.
>(2) Since `#mbox-cells = <0>;`, `spec.args_count` is assigned 0 and
>    `spec.args[]` are untouched.
>(3) Since the controller does not provide `of_xlate`,
>    `of_mbox_index_xlate()` is used instead.
>(4) `idx` is assigned -1 due to the value of `spec.args[0]`.
>(5) Since `mbox->num_chans >= 0` and `idx == -1`, this condition does
>    not filter out this case.
>(6) Out-of-bounds address is returned. Depending on what was left in
>    `spec.args[0]`, it could be an arbitrary(but confined to a specific
>    range) address.
>(7) A function is called with the out-of-bounds address.
>(8) The out-of-bounds address is accessed.

There is no need to write such long commit log to describe the issue
which is easy to understand what it is.

And the subject should be improved:
mailbox: Avoid out-of-band access in of_mbox_index_xlate()

>
>This commit prevents the undefined behavior by checking the array bounds
>and matching the types of the argument for correct filtering.
>
>Signed-off-by: Joonwon Kang <joonwonkang@...gle.com>
>---
> drivers/mailbox/mailbox.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>index 5cd8ae222073..6ed53868c83d 100644
>--- a/drivers/mailbox/mailbox.c
>+++ b/drivers/mailbox/mailbox.c
>@@ -474,9 +474,9 @@ static struct mbox_chan *
> of_mbox_index_xlate(struct mbox_controller *mbox,
> 		    const struct of_phandle_args *sp)
> {
>-	int ind = sp->args[0];
>+	uint32_t ind = sp->args[0];

int should be fine.

> 
>-	if (ind >= mbox->num_chans)
>+	if (sp->args_count < 1 || ind >= mbox->num_chans)
> 		return ERR_PTR(-EINVAL);

Regards
Peng