| 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
| ||
|
Message-ID: <780359a6-5a88-52e4-4404-fd6291b609da@foss.st.com>
Date: Mon, 11 Oct 2021 16:05:37 +0200
From: Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
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 10/9/21 1:42 AM, Bjorn Andersson wrote:
> 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?
>
on virtio backend each side has its endpoint address and have to know the
destination endpoint address. When you use the rpmsg_send() this send a message
to the remote side at the default dest address.
Now if we create the local endpoint at each fopen, how to ensure that the local
address is static?
elle using a dynamic address would imply that each time we open the device we
send this new address to the remote side.
Using the default endpoint of the channel make this simple to use.
>> 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 *.
For the static_ept, your proposal seems to me cleaner, thanks!
For the rpmsg_create_default_ept. The virtio_rpmsg_announce_create and
virtio_rpmsg_announce_destroy are based on the default endpoint.
So to be able to manage ns announcement, seems better to have a default endpoint.
Today something missing in this function is the call of announce_create ops
I was waiting for the integration of the work before sending the fix but i can
include it:
@@ -143,6 +143,7 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct
rpmsg_device *rpdev,
struct rpmsg_channel_info chinfo)
{
struct rpmsg_endpoint *ept;
+ int err;
if (WARN_ON(!rpdev))
return NULL;
@@ -162,6 +163,9 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct
rpmsg_device *rpdev,
rpdev->ept = ept;
rpdev->src = ept->addr;
+ if (ept && rpdev->ops->announce_create)
+ err = rpdev->ops->announce_create(rpdev);
+
return ept;
}
>
>> };
>>
>> 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.
what about -EACCES?
Thanks,
Arnaud
>
> Regards,
> Bjorn
>
>> + return -EPERM;
>> +
>> return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
>> }
>>
>> --
>> 2.17.1
>>
Powered by blists - more mailing lists