linux-kernel - Re: [PATCH] remoteproc: stm32: fix mbox_send

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 May 2021 22:26:52 -0500
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
Cc:     Ohad Ben-Cohen <ohad@...ery.com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH] remoteproc: stm32: fix mbox_send_message call

On Tue 20 Apr 04:19 CDT 2021, Arnaud Pouliquen wrote:

> mbox_send_message is called by passing a local dummy message or
> a function parameter. As the message is queued, it is dereferenced.
> This works because the message field is not used by the stm32 ipcc
> driver, but it is not clean.
> 
> Fix by passing a constant string in all cases.
> 
> The associated comments are removed because rproc should not have to
> deal with the behavior of the mailbox frame.
> 

Didn't we conclude that the mailbox driver doesn't actually dereference
the pointer being passed?

If so I would prefer that you just pass NULL, so that if you in the
future need to pass some actual data it will be easy to distinguish the
old and new case.

Regards,
Bjorn

> Reported-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
> ---
>  drivers/remoteproc/stm32_rproc.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index 7353f9e7e7af..0e8203a432ab 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -474,14 +474,12 @@ static int stm32_rproc_attach(struct rproc *rproc)
>  static int stm32_rproc_detach(struct rproc *rproc)
>  {
>  	struct stm32_rproc *ddata = rproc->priv;
> -	int err, dummy_data, idx;
> +	int err, idx;
>  
>  	/* Inform the remote processor of the detach */
>  	idx = stm32_rproc_mbox_idx(rproc, STM32_MBX_DETACH);
>  	if (idx >= 0 && ddata->mb[idx].chan) {
> -		/* A dummy data is sent to allow to block on transmit */
> -		err = mbox_send_message(ddata->mb[idx].chan,
> -					&dummy_data);
> +		err = mbox_send_message(ddata->mb[idx].chan, "stop");
>  		if (err < 0)
>  			dev_warn(&rproc->dev, "warning: remote FW detach without ack\n");
>  	}
> @@ -493,15 +491,13 @@ static int stm32_rproc_detach(struct rproc *rproc)
>  static int stm32_rproc_stop(struct rproc *rproc)
>  {
>  	struct stm32_rproc *ddata = rproc->priv;
> -	int err, dummy_data, idx;
> +	int err, idx;
>  
>  	/* request shutdown of the remote processor */
>  	if (rproc->state != RPROC_OFFLINE) {
>  		idx = stm32_rproc_mbox_idx(rproc, STM32_MBX_SHUTDOWN);
>  		if (idx >= 0 && ddata->mb[idx].chan) {
> -			/* a dummy data is sent to allow to block on transmit */
> -			err = mbox_send_message(ddata->mb[idx].chan,
> -						&dummy_data);
> +			err = mbox_send_message(ddata->mb[idx].chan, "detach");
>  			if (err < 0)
>  				dev_warn(&rproc->dev, "warning: remote FW shutdown without ack\n");
>  		}
> @@ -556,7 +552,7 @@ static void stm32_rproc_kick(struct rproc *rproc, int vqid)
>  			continue;
>  		if (!ddata->mb[i].chan)
>  			return;
> -		err = mbox_send_message(ddata->mb[i].chan, (void *)(long)vqid);
> +		err = mbox_send_message(ddata->mb[i].chan, "kick");
>  		if (err < 0)
>  			dev_err(&rproc->dev, "%s: failed (%s, err:%d)\n",
>  				__func__, ddata->mb[i].name, err);
> -- 
> 2.17.1
>