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

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

[Oded Gabbay]

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@gmail.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));
 }
 
 static ssize_t soft_reset_cnt_show(struct device *dev,
-- 
2.17.1

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

[Tomer Tayar]

From: Oded Gabbay <oded.gabbay@gmail.com>
Sent: Monday, 8 July 2019 13:44

> 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@gmail.com>

Reviewed-by: Tomer Tayar <ttayar@habana.ai>

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

[Greg KH]

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@gmail.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

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

[Oded Gabbay]

On Mon, Jul 8, 2019 at 2:21 PM Greg KH <gregkh@linuxfoundation.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@gmail.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.

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

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

[Oded Gabbay]

On Mon, Jul 8, 2019 at 2:30 PM Oded Gabbay <oded.gabbay@gmail.com> wrote:
>
> On Mon, Jul 8, 2019 at 2:21 PM Greg KH <gregkh@linuxfoundation.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@gmail.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

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

[Greg KH]

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@gmail.com> wrote:
> >
> > On Mon, Jul 8, 2019 at 2:21 PM Greg KH <gregkh@linuxfoundation.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@gmail.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

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

[Greg KH]

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@linuxfoundation.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@gmail.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.

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.

> 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 :)

thanks,

greg k-h

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@linuxfoundation.org> 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@linuxfoundation.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@gmail.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.
>
> 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

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

[Greg KH]

On Mon, Jul 08, 2019 at 02:51:33PM +0300, Oded Gabbay wrote:
> On Mon, Jul 8, 2019 at 2:43 PM Greg KH <gregkh@linuxfoundation.org> 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@linuxfoundation.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@gmail.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.
> >
> > 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.

But how can you tell if the first one is the "malicious" one and the
second one is "legitimate"?  You do that by the "normal" file
permissions and the like, you don't try to do a "first one wins!" type
of policy, that's crazy :)

> I don't care if one of them duplicates his own FD, right ?

If you are trying to keep someone from having multiple FD per device,
then yes, you should care.  The point is, you can't know.

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

Don't rely on "first one wins!" as a security policy to prevent bad
things from happening.  That's never going to work, let userspace police
these things, as that is the only place you can do it properly (or with
a selinux/apparmor/lsm policy).

> > > 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.

If you run across it again, please let me know.  It's a common
"anti-pattern" that I have been guilty of in the past.

thanks,

greg k-h

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

[Oded Gabbay]

On Mon, Jul 8, 2019 at 3:20 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 08, 2019 at 02:51:33PM +0300, Oded Gabbay wrote:
> > On Mon, Jul 8, 2019 at 2:43 PM Greg KH <gregkh@linuxfoundation.org> 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@linuxfoundation.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@gmail.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.
> > >
> > > 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.
>
> But how can you tell if the first one is the "malicious" one and the
> second one is "legitimate"?  You do that by the "normal" file
> permissions and the like, you don't try to do a "first one wins!" type
> of policy, that's crazy :)

I don't care who is malicious and who is not. Of course I can't count
on "first one wins" to do that...
What I care about, is that two different processes won't be able to
send jobs to the device at the same time. That's it.
But I see your point about not using file-descriptors to enforce this
limitation.
I will change my code, but it will take a bit of time. It's not a
trivial change.

Thanks,
Oded

>
> > I don't care if one of them duplicates his own FD, right ?
>
> If you are trying to keep someone from having multiple FD per device,
> then yes, you should care.  The point is, you can't know.
>
> > Please tell me if my assumption here is correct or not, because this
> > has implications.
>
> Don't rely on "first one wins!" as a security policy to prevent bad
> things from happening.  That's never going to work, let userspace police
> these things, as that is the only place you can do it properly (or with
> a selinux/apparmor/lsm policy).
>
> > > > 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.
>
> If you run across it again, please let me know.  It's a common
> "anti-pattern" that I have been guilty of in the past.
>
> thanks,
>
> greg k-h