From: Andre Przywara <andre.przywara@arm.com> To: Samuel Holland <samuel@sholland.org>, Maxime Ripard <maxime.ripard@free-electrons.com>, Chen-Yu Tsai <wens@csie.org>, Jassi Brar <jassisinghbrar@gmail.com>, Rob Herring <robh+dt@kernel.org> Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org Subject: Re: [PATCH 2/3] mailbox: Avoid NULL dereference in mbox_chan_received_data Date: Wed, 28 Feb 2018 17:17:13 +0000 [thread overview] Message-ID: <d4778c2e-9d3b-d1c4-429f-6f006c78deaf@arm.com> (raw) In-Reply-To: <20180228022714.30068-3-samuel@sholland.org> Hi, On 28/02/18 02:27, Samuel Holland wrote: > If a reception IRQ is pending when a mailbox channel is shut down (for > example, if the controller uses threaded interrupts), it is possible for > mbox_chan_received_data to be called while chan->cl is NULL. > > This was found while developing a mailbox controller driver for use with > SCPI. The SCPI protocol driver frees its mailbox channel during probing > if the SCP firmware does not respond within a specified timeout. In this > case, if the SCP firmware takes slightly too long to respond, > mbox_chan_received_data races with mbox_free_channel clearing chan->cl. > > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > drivers/mailbox/mailbox.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > index 674b35f402f5..a0258d8672d5 100644 > --- a/drivers/mailbox/mailbox.c > +++ b/drivers/mailbox/mailbox.c > @@ -152,9 +152,11 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer) > */ > void mbox_chan_received_data(struct mbox_chan *chan, void *mssg) > { > + struct mbox_client *cl = READ_ONCE(chan->cl); > + > /* No buffering the received data */ > - if (chan->cl->rx_callback) > - chan->cl->rx_callback(chan->cl, mssg); > + if (cl && cl->rx_callback) > + cl->rx_callback(cl, mssg); I don't think this is the proper fix. This sounds like we should have a lock here. If mbox_free_channel now frees or clears chan->cl between the READ_ONCE and the second part of the "&&", we will have a use-after-free. If it's being cleared after the comparison, we will end up with a NULL pointer dereference again, IIUC. Or am I missing something? Cheers, Andre. > } > EXPORT_SYMBOL_GPL(mbox_chan_received_data); > >
WARNING: multiple messages have this Message-ID (diff)
From: andre.przywara@arm.com (Andre Przywara) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/3] mailbox: Avoid NULL dereference in mbox_chan_received_data Date: Wed, 28 Feb 2018 17:17:13 +0000 [thread overview] Message-ID: <d4778c2e-9d3b-d1c4-429f-6f006c78deaf@arm.com> (raw) In-Reply-To: <20180228022714.30068-3-samuel@sholland.org> Hi, On 28/02/18 02:27, Samuel Holland wrote: > If a reception IRQ is pending when a mailbox channel is shut down (for > example, if the controller uses threaded interrupts), it is possible for > mbox_chan_received_data to be called while chan->cl is NULL. > > This was found while developing a mailbox controller driver for use with > SCPI. The SCPI protocol driver frees its mailbox channel during probing > if the SCP firmware does not respond within a specified timeout. In this > case, if the SCP firmware takes slightly too long to respond, > mbox_chan_received_data races with mbox_free_channel clearing chan->cl. > > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > drivers/mailbox/mailbox.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > index 674b35f402f5..a0258d8672d5 100644 > --- a/drivers/mailbox/mailbox.c > +++ b/drivers/mailbox/mailbox.c > @@ -152,9 +152,11 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer) > */ > void mbox_chan_received_data(struct mbox_chan *chan, void *mssg) > { > + struct mbox_client *cl = READ_ONCE(chan->cl); > + > /* No buffering the received data */ > - if (chan->cl->rx_callback) > - chan->cl->rx_callback(chan->cl, mssg); > + if (cl && cl->rx_callback) > + cl->rx_callback(cl, mssg); I don't think this is the proper fix. This sounds like we should have a lock here. If mbox_free_channel now frees or clears chan->cl between the READ_ONCE and the second part of the "&&", we will have a use-after-free. If it's being cleared after the comparison, we will end up with a NULL pointer dereference again, IIUC. Or am I missing something? Cheers, Andre. > } > EXPORT_SYMBOL_GPL(mbox_chan_received_data); > >
next prev parent reply other threads:[~2018-02-28 17:17 UTC|newest] Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-28 2:27 [PATCH 0/3] Allwinner sunxi message box support Samuel Holland 2018-02-28 2:27 ` Samuel Holland 2018-02-28 2:27 ` [PATCH 1/3] dt-bindings: Add a binding for the sunxi message box Samuel Holland 2018-02-28 2:27 ` Samuel Holland 2018-02-28 8:28 ` Maxime Ripard 2018-02-28 8:28 ` Maxime Ripard 2018-02-28 17:17 ` Andre Przywara 2018-02-28 17:17 ` Andre Przywara 2018-03-01 10:03 ` Maxime Ripard 2018-03-01 10:03 ` Maxime Ripard 2018-02-28 17:52 ` Samuel Holland 2018-02-28 17:52 ` Samuel Holland 2018-02-28 2:27 ` [PATCH 2/3] mailbox: Avoid NULL dereference in mbox_chan_received_data Samuel Holland 2018-02-28 2:27 ` Samuel Holland 2018-02-28 17:17 ` Andre Przywara [this message] 2018-02-28 17:17 ` Andre Przywara 2018-03-01 13:32 ` Jassi Brar 2018-03-01 13:32 ` Jassi Brar 2018-02-28 2:27 ` [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver Samuel Holland 2018-02-28 2:27 ` Samuel Holland 2018-02-28 8:32 ` Maxime Ripard 2018-02-28 8:32 ` Maxime Ripard 2018-02-28 17:19 ` Samuel Holland 2018-02-28 17:19 ` Samuel Holland 2018-03-01 10:32 ` Maxime Ripard 2018-03-01 10:32 ` Maxime Ripard 2018-03-01 11:32 ` Andre Przywara 2018-03-01 11:32 ` Andre Przywara 2018-03-01 11:51 ` Maxime Ripard 2018-03-01 11:51 ` Maxime Ripard 2018-02-28 9:16 ` Jassi Brar 2018-02-28 9:16 ` Jassi Brar 2018-02-28 17:51 ` Samuel Holland 2018-02-28 17:51 ` Samuel Holland 2018-02-28 18:14 ` Jassi Brar 2018-02-28 18:14 ` Jassi Brar 2018-02-28 18:14 ` Jassi Brar 2018-02-28 18:56 ` Samuel Holland 2018-02-28 18:56 ` Samuel Holland 2018-02-28 18:56 ` Samuel Holland 2018-03-01 5:22 ` Jassi Brar 2018-03-01 5:22 ` Jassi Brar 2018-03-01 5:22 ` Jassi Brar 2018-02-28 8:24 ` [PATCH 0/3] Allwinner sunxi message box support Maxime Ripard 2018-02-28 8:24 ` Maxime Ripard 2018-02-28 17:18 ` Samuel Holland 2018-02-28 17:18 ` Samuel Holland 2018-03-01 10:28 ` Maxime Ripard 2018-03-01 10:28 ` Maxime Ripard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d4778c2e-9d3b-d1c4-429f-6f006c78deaf@arm.com \ --to=andre.przywara@arm.com \ --cc=devicetree@vger.kernel.org \ --cc=jassisinghbrar@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maxime.ripard@free-electrons.com \ --cc=robh+dt@kernel.org \ --cc=samuel@sholland.org \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.