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

[Oded Gabbay]

On Mon, Jul 8, 2019 at 2:43 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Jul 08, 2019 at 02:30:13PM +0300, Oded Gabbay wrote:
> > On Mon, Jul 8, 2019 at 2:21 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > >  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.
>
> But the issue is that you can't "force" this from the kernel side at
> all.  Trying to catch this at open() time only catches the obvious
> processes.
>
> As I said, how do you check for:
>         fd = open(...);
>         fd_new = dup(fd);
>
>         write(fd, ...);
>         write(fd_new, ...);
>
> or, pass the fd across a socket?  Or other fun ways of sending file
> descriptors around a system.
>
> You have to trust userspace here, sorry.  If someone wants to do
> multiple accesses, they can, but again, they deserve the pieces when
> things fall apart.

I see what you are saying, but from *security* perspective, I don't
think I really care about the scenarios above, right ?
Because that would mean a user duplicated his *own* fd. Sure, things
won't work for him, but what do I care about that, as you rightly
said.
I'm only concerned about the security risk, where there is a
legitimate user and a malicious one. Because I can't isolate between
them, I want to be able to allow only one of them to run.
I don't care if one of them duplicates his own FD, right ?

Please tell me if my assumption here is correct or not, because this
has implications.

>
> > 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.
> >
> > 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.
>
> What drivers show the number of open file descriptors?  Time to go
> delete them as well :)
hehe
I tried to grep for it but I couldn't find any. Strange because I was
sure I saw this in some driver.
As I said, I don't really care about it. I will delete this to prevent
further errors.

Oded

>
> thanks,
>
> greg k-h