linux-kernel - Re: [PATCH] habanalabs: use correct variable to show fd open counter

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190708122103.GB19624@kroah.com>
Date:   Mon, 8 Jul 2019 14:21:03 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Oded Gabbay <oded.gabbay@...il.com>
Cc:     "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
        Omer Shpigelman <oshpigelman@...ana.ai>,
        Tomer Tayar <ttayar@...ana.ai>
Subject: Re: [PATCH] habanalabs: use correct variable to show fd open counter

On Mon, Jul 08, 2019 at 02:37:45PM +0300, Oded Gabbay wrote:
> On Mon, Jul 8, 2019 at 2:30 PM Oded Gabbay <oded.gabbay@...il.com> wrote:
> >
> > On Mon, Jul 8, 2019 at 2:21 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > >
> > > On Mon, Jul 08, 2019 at 01:43:55PM +0300, Oded Gabbay wrote:
> > > > The current code checks if the user context pointer is NULL or not to
> > > > display the number of open file descriptors of a device. However, that
> > > > variable (user_ctx) will eventually go away as the driver will support
> > > > multiple processes. Instead, the driver can use the atomic counter of
> > > > the open file descriptors which the driver already maintains.
> > > >
> > > > Signed-off-by: Oded Gabbay <oded.gabbay@...il.com>
> > > > ---
> > > >  drivers/misc/habanalabs/sysfs.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/misc/habanalabs/sysfs.c b/drivers/misc/habanalabs/sysfs.c
> > > > index 25eb46d29d88..881be19b5fad 100644
> > > > --- a/drivers/misc/habanalabs/sysfs.c
> > > > +++ b/drivers/misc/habanalabs/sysfs.c
> > > > @@ -356,7 +356,7 @@ static ssize_t write_open_cnt_show(struct device *dev,
> > > >  {
> > > >       struct hl_device *hdev = dev_get_drvdata(dev);
> > > >
> > > > -     return sprintf(buf, "%d\n", hdev->user_ctx ? 1 : 0);
> > > > +     return sprintf(buf, "%d\n", atomic_read(&hdev->fd_open_cnt));
> > > >  }
> > >
> > > Odds are, this means nothing, as it doesn't get touched if the file
> > > descriptor is duped or sent to another process.
> > >
> > > Why do you care about the number of open files?  Whenever someone tries
> > > to do this type of logic, it is almost always wrong, just let userspace
> > > do what it wants to do, and if wants to open something twice, then it
> > > gets to keep the pieces when things break.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > I care about the number of open file descriptors because I can't let
> > multiple processes run simultaneously on my device, as we still don't
> > have the code to do proper isolation between the processes, in regard
> > of memory accesses on our device memory and by using our DMA engine.
> > Basically, it's a security hole. If you want, I can explain more in
> > length on this issue.
> >
> > We have the H/W infrastructure for that, using MMU and multiple
> > address space IDs (ASID), but we didn't write the code yet in the
> > driver, as that is a BIG feature but it wasn't requested by anyone
> > yet.
> Let me amend the above statement:
> We have the H/W infrastructure for that in GOYA. In GAUDI (I don't
> know if you saw the press release), we don't have it, as the use-case
> of that asic (training) doesn't require multiple processes support.
> Therefore, when I will upstream that ASIC code, I will have to always
> prevent multiple processes from opening file descriptors at the same
> time, due to the above security concerns.

Again, let userspace/LSM enforce those policies, you can't do it in your
driver successfully at all.

thanks,

greg k-h