On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky [off-list ref] wrote:

On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote:

quoted

Leon Romanovsky [off-list ref] 于2021年9月28日周二 上午9:28写道：

quoted

On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:

quoted

Leon Romanovsky [off-list ref] 于2021年9月27日周一 下午2:23写道：

quoted

On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:

quoted

Allowing these characters in sessname can lead to unexpected results,
particularly because / is used as a separator between files in a path,
and . points to the current directory.

Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Reviewed-by: Gioh Kim <redacted>
Reviewed-by: Aleksei Marov <redacted>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
 2 files changed, 11 insertions(+)

It will be safer if you check for only allowed symbols and disallow
everything else. Check for: a-Z, 0-9 and "-".

Hi Leon,

Thanks for your suggestions.
The reasons we choose to do disallow only '/' and '.':
1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.

So you need to add all possible protections and checks that VFS has to allow "random" name.

It's only about sysfs here, as we use sessname to create dir in sysfs,
and I checked the code, it allows any 8-bit set, and convert '/' to
'!', see https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L299

quoted

quoted

2 matching for 2 characters is faster than checking all the allowed
symbols during session establishment.

Extra CPU cycles won't make any difference here.

As we can have hundreds of sessions, in the end, it matters.

Your rtrs_clt_open() function is far from being optimized for
performance. It allocates memory, iterates over all paths, creates
sysfs and kobject.

So no, it doesn't matter here.

Let me reiterate, why do we want to further slow it down, what do you
anticipate if we only do the disallow approach
as we do it now?

Thanks!

Thanks

quoted

Thanks

quoted

quoted

3 we do use hostnameA@hostnameB for production usage right now, we
don't want to break the user space.

You can add "@" into the list of accepted symbols.

quoted

I hope this makes sense to you.

Regards!

quoted

quoted

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index bc8824b4ee0d..15c0077dd27e 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c

@@ -2788,6 +2788,12 @@ struct rtrs_clt *rtrs_clt_open(struct rtrs_clt_ops *ops,
      struct rtrs_clt *clt;
      int err, i;

+     if (strchr(sessname, '/') || strchr(sessname, '.')) {
+             pr_err("sessname cannot contain / and .\n");
+             err = -EINVAL;
+             goto out;
+     }
+
      clt = alloc_clt(sessname, paths_num, port, pdu_sz, ops->priv,
                      ops->link_ev,
                      reconnect_delay_sec,

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 078a1cbac90c..7df71f8cf149 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c

@@ -803,6 +803,11 @@ static int process_info_req(struct rtrs_srv_con *con,
              return err;
      }

+     if (strchr(msg->sessname, '/') || strchr(msg->sessname, '.')) {
+             rtrs_err(s, "sessname cannot contain / and .\n");
+             return -EINVAL;
+     }
+
      if (exist_sessname(sess->srv->ctx,
                         msg->sessname, &sess->srv->paths_uuid)) {
              rtrs_err(s, "sessname is duplicated: %s\n", msg->sessname);
--

2.25.1