| Message ID | 20210922125333.351454-7-haris.iqbal@ionos.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Misc update for RTRS | expand |
On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > 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 <gi-oh.kim@ionos.com> > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > --- > 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 "-". Thanks > > 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 >
Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > 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 <gi-oh.kim@ionos.com> > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > --- > > 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. 2 matching for 2 characters is faster than checking all the allowed symbols during session establishment. 3 we do use hostnameA@hostnameB for production usage right now, we don't want to break the user space. I hope this makes sense to you. Regards! > > > > > 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 > >
On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote: > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > > 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 <gi-oh.kim@ionos.com> > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > > --- > > > 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. > 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. > 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. > > I hope this makes sense to you. > > Regards! > > > > > > > > > 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 > > >
Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道： > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote: > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > > > 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 <gi-oh.kim@ionos.com> > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > > > --- > > > > 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 > > > 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. Thanks > > > 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. > > > > > I hope this makes sense to you. > > > > Regards! > > > > > > > > > > > > > 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 > > > >
On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote: > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道： > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote: > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > > > > 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 <gi-oh.kim@ionos.com> > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > > > > --- > > > > > 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 > > > > > 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. Thanks > > Thanks > > > > > 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. > > > > > > > > I hope this makes sense to you. > > > > > > Regards! > > > > > > > > > > > > > > > > > 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 > > > > >
On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote: > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote: > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道： > > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote: > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > > > > > 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 <gi-oh.kim@ionos.com> > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > > > > > --- > > > > > > 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 > > > > > > > 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 > > > > > Thanks > > > > > > > 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. > > > > > > > > > > > I hope this makes sense to you. > > > > > > > > Regards! > > > > > > > > > > > > > > > > > > > > > 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 > > > > > >
On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote: > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote: > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道： > > > > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote: > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > > > > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > > > > > > 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 <gi-oh.kim@ionos.com> > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > > > > > > --- > > > > > > > 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 > > > > > > > > > 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? It is common practice to sanitize user input and explicitly allow known good input, instead of relying on deny of bad input. We don't know the future and can't be sure that "deny" is actually closed all holes. Thanks
On Thu, Sep 30, 2021 at 9:01 AM Leon Romanovsky <leon@kernel.org> wrote: > > On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote: > > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote: > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道： > > > > > > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote: > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > > > > > > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > > > > > > > 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 <gi-oh.kim@ionos.com> > > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > > > > > > > --- > > > > > > > > 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 > > > > > > > > > > > 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? > > It is common practice to sanitize user input and explicitly allow known > good input, instead of relying on deny of bad input. We don't know the > future and can't be sure that "deny" is actually closed all holes. Thanks for the clarification, but still what kind of holes do you have in mind, the input string length is already checked and it's not duplicated with other sessname. and sysfs does allow all 8 bit set IIUC. Thanks! > Thanks
On Thu, Sep 30, 2021 at 09:10:33AM +0200, Jinpu Wang wrote: > On Thu, Sep 30, 2021 at 9:01 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote: > > > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote: > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道： > > > > > > > > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote: > > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > > > > > > > > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > > > > > > > > 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 <gi-oh.kim@ionos.com> > > > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > > > > > > > > --- > > > > > > > > > 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 > > > > > > > > > > > > > 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? > > > > It is common practice to sanitize user input and explicitly allow known > > good input, instead of relying on deny of bad input. We don't know the > > future and can't be sure that "deny" is actually closed all holes. > > Thanks for the clarification, but still what kind of holes do you have > in mind, the input string length is already checked and it's > not duplicated with other sessname. and sysfs does allow all 8 bit set IIUC. As an example, symbols like "/" and "\". > > Thanks! > > > Thanks
On Thu, Sep 30, 2021 at 9:53 AM Leon Romanovsky <leon@kernel.org> wrote: > > On Thu, Sep 30, 2021 at 09:10:33AM +0200, Jinpu Wang wrote: > > On Thu, Sep 30, 2021 at 9:01 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote: > > > > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote: > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道： > > > > > > > > > > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote: > > > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > > > > > > > > > > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > > > > > > > > > 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 <gi-oh.kim@ionos.com> > > > > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > > > > > > > > > --- > > > > > > > > > > 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 > > > > > > > > > > > > > > > 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? > > > > > > It is common practice to sanitize user input and explicitly allow known > > > good input, instead of relying on deny of bad input. We don't know the > > > future and can't be sure that "deny" is actually closed all holes. > > > > Thanks for the clarification, but still what kind of holes do you have > > in mind, the input string length is already checked and it's > > not duplicated with other sessname. and sysfs does allow all 8 bit set IIUC. > > As an example, symbols like "/" and "\". "/" aside, we already disable it. I did a test, there is no problem to use "\" as sysfs name. Thanks! > > > > > Thanks! > > > > > Thanks
On Fri, Oct 01, 2021 at 02:40:24PM +0200, Jinpu Wang wrote: > On Thu, Sep 30, 2021 at 9:53 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > On Thu, Sep 30, 2021 at 09:10:33AM +0200, Jinpu Wang wrote: > > > On Thu, Sep 30, 2021 at 9:01 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote: > > > > > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > > > > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote: > > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道： > > > > > > > > > > > > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote: > > > > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > > > > > > > > > > > > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > > > > > > > > > > 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 <gi-oh.kim@ionos.com> > > > > > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > > > > > > > > > > --- > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > 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? > > > > > > > > It is common practice to sanitize user input and explicitly allow known > > > > good input, instead of relying on deny of bad input. We don't know the > > > > future and can't be sure that "deny" is actually closed all holes. > > > > > > Thanks for the clarification, but still what kind of holes do you have > > > in mind, the input string length is already checked and it's > > > not duplicated with other sessname. and sysfs does allow all 8 bit set IIUC. > > > > As an example, symbols like "/" and "\". > "/" aside, we already disable it. > I did a test, there is no problem to use "\" as sysfs name. It say nothing about future. Please change your implementation to accept only valid symbols and improve connection performance in other places. Thanks > > Thanks! > > > > > > > > Thanks! > > > > > > > Thanks
On Fri, Oct 1, 2021 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote: > > On Fri, Oct 01, 2021 at 02:40:24PM +0200, Jinpu Wang wrote: > > On Thu, Sep 30, 2021 at 9:53 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > On Thu, Sep 30, 2021 at 09:10:33AM +0200, Jinpu Wang wrote: > > > > On Thu, Sep 30, 2021 at 9:01 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > > > On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote: > > > > > > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > > > > > > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote: > > > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道： > > > > > > > > > > > > > > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote: > > > > > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道： > > > > > > > > > > > > > > > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote: > > > > > > > > > > > > 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 <gi-oh.kim@ionos.com> > > > > > > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com> > > > > > > > > > > > > --- > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > 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? > > > > > > > > > > It is common practice to sanitize user input and explicitly allow known > > > > > good input, instead of relying on deny of bad input. We don't know the > > > > > future and can't be sure that "deny" is actually closed all holes. > > > > > > > > Thanks for the clarification, but still what kind of holes do you have > > > > in mind, the input string length is already checked and it's > > > > not duplicated with other sessname. and sysfs does allow all 8 bit set IIUC. > > > > > > As an example, symbols like "/" and "\". > > "/" aside, we already disable it. > > I did a test, there is no problem to use "\" as sysfs name. > > It say nothing about future. > > Please change your implementation to accept only valid > symbols and improve connection performance in other places. > > Thanks Sorry, I'm really not convinced, why should we only do allowing, not disabling in this case? Jason, what's your suggestion? Thanks > > > > > Thanks! > > > > > > > > > > > Thanks! > > > > > > > > > Thanks
On Mon, Oct 04, 2021 at 07:41:08AM +0200, Jinpu Wang wrote: > Sorry, I'm really not convinced, why should we only do allowing, not > disabling in this case? > > Jason, what's your suggestion? At least from a correctness perspective only the characters / and \0 are forbidden, as well as any existing filenames like . and .. Generally it is not a great idea to restrict things too much because it can block the full use of UTF-8 for non-English languages. However, I also think the sysfs in this driver is far too elaborate, the time to complain was before this was all merged unfortunately. Given that it is a simple enough bug fix it may as well go ahead as-is. Jason
On Mon, Oct 4, 2021 at 8:47 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Mon, Oct 04, 2021 at 07:41:08AM +0200, Jinpu Wang wrote: > > Sorry, I'm really not convinced, why should we only do allowing, not > > disabling in this case? > > > > Jason, what's your suggestion? > > At least from a correctness perspective only the characters / and \0 > are forbidden, as well as any existing filenames like . and .. > > Generally it is not a great idea to restrict things too much because > it can block the full use of UTF-8 for non-English languages. > > However, I also think the sysfs in this driver is far too elaborate, > the time to complain was before this was all merged > unfortunately. Given that it is a simple enough bug fix it may as well > go ahead as-is. > > Jason Thanks Jason and Leon.
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);