Messages in this thread | | | From | Oded Gabbay <> | Date | Mon, 8 Jul 2019 14:51:33 +0300 | Subject | Re: [PATCH] habanalabs: use correct variable to show fd open counter |
| |
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
| |