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
| ||
|
Date: Fri, 8 Oct 2021 16:42:53 -0700 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, julien.massot@....bzh Subject: Re: [PATCH v4 3/4] rpmsg: char: Add possibility to use default endpoint of the rpmsg device. On Mon 12 Jul 06:18 PDT 2021, Arnaud Pouliquen wrote: > Current implementation create/destroy a new endpoint on each > rpmsg_eptdev_open/rpmsg_eptdev_release calls. > > For a rpmsg device created by the NS announcement mechanism we need to > use a unique static endpoint that is the default rpmsg device endpoint > associated to the channel. > Why do you need this endpoint associated with the channel? Afaict the read/write operations still operate on eptdev->ept, so who does use the default endpoint for the device? > This patch prepares the introduction of a rpmsg channel device for the > char device. The rpmsg channel device will require a default endpoint to > communicate to the remote processor. > > Add the static_ept field in rpmsg_eptdev structure. This boolean > determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call. > > - If static_ept == false: > Use the legacy behavior by creating a new endpoint each time > rpmsg_eptdev_open is called and release it when rpmsg_eptdev_release > is called on /dev/rpmsgX device open/close. > > - If static_ept == true: > use the rpmsg device default endpoint for the communication. > - Address the update of _rpmsg_chrdev_eptdev_create in e separate patch for readability. > > Add protection in rpmsg_eptdev_ioctl to prevent to destroy a default endpoint. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@...s.st.com> > Reviewed-by: Mathieu Poirier <mathieu.poirier@...aro.org> > Tested-by: Julien Massot <julien.massot@....bzh> > --- > drivers/rpmsg/rpmsg_char.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > index 50b7d4b00175..bd728d90ba4c 100644 > --- a/drivers/rpmsg/rpmsg_char.c > +++ b/drivers/rpmsg/rpmsg_char.c > @@ -45,6 +45,8 @@ static DEFINE_IDA(rpmsg_minor_ida); > * @queue_lock: synchronization of @queue operations > * @queue: incoming message queue > * @readq: wait object for incoming queue > + * @static_ept: specify if the endpoint has to be created at each device opening or > + * if the default endpoint should be used. > */ > struct rpmsg_eptdev { > struct device dev; > @@ -59,6 +61,8 @@ struct rpmsg_eptdev { > spinlock_t queue_lock; > struct sk_buff_head queue; > wait_queue_head_t readq; > + > + bool static_ept; I think you can skip rpmsg_create_default_ept() if you just make this struct rpmsg_endpoint *. > }; > > int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) > @@ -116,7 +120,15 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp) > > get_device(dev); > > - ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo); > + /* > + * If the static_ept is set to true, the rpmsg device default endpoint is used. > + * Else a new endpoint is created on open that will be destroyed on release. > + */ > + if (eptdev->static_ept) > + ept = rpdev->ept; This would be: if (eptdev->static_ept) ept = eptdev->static_ept; > + else > + ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo); > + > if (!ept) { > dev_err(dev, "failed to open %s\n", eptdev->chinfo.name); > put_device(dev); > @@ -137,7 +149,8 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp) > /* Close the endpoint, if it's not already destroyed by the parent */ > mutex_lock(&eptdev->ept_lock); > if (eptdev->ept) { > - rpmsg_destroy_ept(eptdev->ept); > + if (!eptdev->static_ept) > + rpmsg_destroy_ept(eptdev->ept); > eptdev->ept = NULL; > } > mutex_unlock(&eptdev->ept_lock); > @@ -264,6 +277,10 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd, > if (cmd != RPMSG_DESTROY_EPT_IOCTL) > return -EINVAL; > > + /* Don't allow to destroy a default endpoint. */ > + if (eptdev->ept == eptdev->rpdev->ept) And this would be if: if (eptdev->static_ept) return -EPERM; Wouldn't -EINVAL or something be better than -EPERM when you try to destroy one of these endpoints? It's not that we don't have permission, it's that it's not a valid operation on this object. Regards, Bjorn > + return -EPERM; > + > return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL); > } > > -- > 2.17.1 >
Powered by blists - more mailing lists