linux-kernel - Re: [PATCHv3] remoteproc: Add STE modem driver for remoteproc

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, 20 Sep 2012 13:08:20 +0300
From:	Ohad Ben-Cohen <ohad@...ery.com>
To:	sjur.brandeland@...ricsson.com
Cc:	Sjur Brændeland <sjurbren@...il.com>,
	linux-kernel@...r.kernel.org,
	Linus Walleij <linus.walleij@...aro.org>,
	Alan Cox <alan@...rguk.ukuu.org.uk>
Subject: Re: [PATCHv3] remoteproc: Add STE modem driver for remoteproc

Hi Sjur,

On Wed, Sep 19, 2012 at 7:39 PM,  <sjur.brandeland@...ricsson.com> wrote:
> From: Sjur Brændeland <sjur.brandeland@...ricsson.com>
>
> Add support for the STE modem shared memory driver.
> This driver hooks into the remoteproc framework
> in order to manage configuration and the virtio
> devices.
>
> This driver adds custom firmware handlers, because
> STE modem uses a custom firmware layout.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@...ricsson.com>
> cc: Linus Walleij <linus.walleij@...aro.org>
> cc: Alan Cox <alan@...rguk.ukuu.org.uk>
> ---
> Changes since v3:

Looks good, thanks.

I have just one comment/question:

> +/* STE modem device is unregistered */
> +static int sproc_drv_remove(struct platform_device *pdev)
> +{
> +       struct ste_modem_device *mdev =
> +               container_of(pdev, struct ste_modem_device, pdev);
> +       struct sproc *sproc = mdev->drv_data;
> +
> +       sproc_dbg(sproc, "remove ste-modem\n");
> +
> +       /* Unregister as remoteproc device */
> +       rproc_del(sproc->rproc);
> +       rproc_put(sproc->rproc);
> +
> +       mdev->drv_data = NULL;
> +       mdev->drv_ops = NULL;
> +       return 0;
> +}
> +
> +struct ste_modem_drv_ops sproc_drv_ops = {
> +       .kick = sproc_kick_callback
> +};
> +
> +/* Handle probe of a modem device */
> +static int sproc_probe(struct platform_device *pdev)
> +{
...
> +       mdev->drv_data = sproc;
> +
> +       /* Provide callbacks to modem device */
> +       mdev->drv_ops = &sproc_drv_ops;

Implicitly providing the modem with drv_ops and drv_data in this
manner feels racy and somewhat error prone.

E.g., on remove these members are set to NULL, under the assumption
that the modem won't invoke sproc_kick_callback or access sproc
anymore, but this doesn't feel safe.

Any chance you can add an explicit registration method to
ste_modem_dev_ops, with which you'll be able to explicitly set/unset
sproc_kick_callback and sproc ?

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/