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: <CAFCwf10Siysrdd61itf0stM_8S8UXmzROtAZmV1eJzfC3p7UXg@mail.gmail.com>
Date:   Mon, 8 Jul 2019 14:37:45 +0300
From:   Oded Gabbay <oded.gabbay@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
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 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.

Oded

>
> So the current solution is to block the ability to open multiple file
> descriptors.
>
> Regarding this specific sysfs property, I don't really care about it.
> I simply saw that it is shown in other drivers and I thought it may be
> nice for a system admin utility to show it.
>
> Thanks,
> Oded