linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jinpu Wang <jinpu.wang@ionos.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Jack Wang <xjtuwjp@gmail.com>,
	Md Haris Iqbal <haris.iqbal@ionos.com>,
	RDMA mailing list <linux-rdma@vger.kernel.org>,
	Bart Van Assche <bvanassche@acm.org>,
	Doug Ledford <dledford@redhat.com>,
	Jason Gunthorpe <jgg@ziepe.ca>, Gioh Kim <gi-oh.kim@ionos.com>,
	Aleksei Marov <aleksei.marov@ionos.com>
Subject: Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .
Date: Thu, 30 Sep 2021 09:10:33 +0200	[thread overview]
Message-ID: <CAMGffE=NP-iNcAQyVF57tbeJ1QcyMt7=savh=5BLxaC9TuAkTw@mail.gmail.com> (raw)
In-Reply-To: <YVVgunT1hSIzu1tA@unreal>

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

  reply	other threads:[~2021-09-30  7:10 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-22 12:53 [PATCH for-next 0/7] Misc update for RTRS Md Haris Iqbal
2021-09-22 12:53 ` [PATCH for-next 1/7] RDMA/rtrs: Use sysfs_emit instead of s*printf function for sysfs show Md Haris Iqbal
2021-09-22 12:53 ` [PATCH for-next 2/7] RDMA/rtrs: Remove len parameter from helper print functions of sysfs Md Haris Iqbal
2021-09-22 12:53 ` [PATCH for-next 3/7] RDMA/rtrs: Fix warning when use poll mode on client side Md Haris Iqbal
2021-09-22 12:53 ` [PATCH for-next 4/7] RDMA/rtrs: Replace duplicate check with is_pollqueue helper Md Haris Iqbal
2021-09-22 12:53 ` [PATCH for-next 5/7] RDMA/rtrs: Introduce destroy_cq helper Md Haris Iqbal
2021-09-22 12:53 ` [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and Md Haris Iqbal
2021-09-27 12:22   ` Leon Romanovsky
2021-09-28  7:08     ` Jack Wang
2021-09-28  7:28       ` Leon Romanovsky
2021-09-29  7:00         ` Jack Wang
2021-09-29 12:04           ` Leon Romanovsky
2021-09-30  6:03             ` Jinpu Wang
2021-09-30  7:01               ` Leon Romanovsky
2021-09-30  7:10                 ` Jinpu Wang [this message]
2021-09-30  7:53                   ` Leon Romanovsky
2021-10-01 12:40                     ` Jinpu Wang
2021-10-01 14:56                       ` Leon Romanovsky
2021-10-04  5:41                         ` Jinpu Wang
2021-10-04 18:47                           ` Jason Gunthorpe
2021-10-04 21:05                             ` Jinpu Wang
2021-09-22 12:53 ` [PATCH for-next 7/7] RDMA/rtrs-clt: Follow "one entry one value" rule for IO migration stats Md Haris Iqbal
2021-10-04 19:49 ` [PATCH for-next 0/7] Misc update for RTRS Jason Gunthorpe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMGffE=NP-iNcAQyVF57tbeJ1QcyMt7=savh=5BLxaC9TuAkTw@mail.gmail.com' \
    --to=jinpu.wang@ionos.com \
    --cc=aleksei.marov@ionos.com \
    --cc=bvanassche@acm.org \
    --cc=dledford@redhat.com \
    --cc=gi-oh.kim@ionos.com \
    --cc=haris.iqbal@ionos.com \
    --cc=jgg@ziepe.ca \
    --cc=leon@kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=xjtuwjp@gmail.com \
    --subject='Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).