Re: [PATCH 1/4] gpio: userspace ABI for reading/writing GPIO lines

[Alexander Stein <alexander.stein@systec-electronic.com>]

On Wednesday 29 June 2016 11:05:49, Linus Walleij wrote:
> On Wed, Jun 29, 2016 at 10:53 AM, Alexander Stein
> 
> <alexander.stein@systec-electronic.com> wrote:
> > On Thursday 02 June 2016 13:51:26, Linus Walleij wrote:
> >> +     fd = anon_inode_getfd("gpio-linehandle",
> >> +                           &linehandle_fileops,
> >> +                           lh,
> >> +                           O_RDONLY | O_CLOEXEC);
> > 
> > When will linehandle_release actually be called? When we explicitely call
> > close(req.fd) or when application exits and all FDs will be closed anyway?
> 
> As far as I understand: both.
> 
> And I don't think anything else makes sense? If an application is
> terminated, the operating environment will close all open file handles as
> far as I know, at least that is how I always thought it works.
> 
> But hey, it's userspace so what do I know...
> 
> Anyways if it doesn't work like so, there are a bunch of subsystems in
> the kernel using it so they would all have severe problems.

Fine, just wanted to make sure.

> >> +     if (fd < 0) {
> >> +             ret = fd;
> >> +             goto out_free_descs;
> >> +     }
> >> +
> >> +     handlereq.fd = fd;
> >> +     if (copy_to_user(ip, &handlereq, sizeof(handlereq)))
> >> +             return -EFAULT;
> > 
> > If I'm right above doesn't that then leak lh until application eventually
> > exits? Userspace won't receive the newly created fd. The same would apply
> > to patch 3/4.
> 
> I don't understand....
> 
> The userspace application reads the .fd fields of the handle request after
> issuing the ioctl() and it then contains the new file handle.

There is no valid .fd if copy_to_user() fails and immediately returning
-EFAULT. Thus the previously kzalloc'ed lh (well anything linehandle_release 
frees) is unaccessible. Userspace has no .fd for neither any ioctl nor for 
calling close(). This allocated memory will remain unavailable until the 
process exits. I think something like this should be added:

> @@ -486,8 +486,10 @@ static int linehandle_create(struct gpio_device *gdev,
> void __user *ip)> 
>  	}
>  	
>  	handlereq.fd = fd;
> 
> -	if (copy_to_user(ip, &handlereq, sizeof(handlereq)))
> -		return -EFAULT;
> +	if (copy_to_user(ip, &handlereq, sizeof(handlereq))) {
> +		ret = -EFAULT;
> +		goto out_free_descs;
> +	}
> 
>  	dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n",
>  	
>  		lh->numdescs);

I hope this could make my concerns more clear. Opinions?

Best regards,
Alexander