[PATCH 0/2] Export receive_fd() to modules and do some cleanups

[PATCH 0/2] Export receive_fd() to modules and do some cleanups

[Xie Yongji]

This series starts from Christian's comments on the series[1].
We'd like to export receive_fd() which can not only be used by
our module in the series[1] but also allow further cleanups
like patch 2 does.

Now this series is based on Christoph's patch[2].

[1] https://lore.kernel.org/linux-fsdevel/20210331080519.172-1-xieyongji@bytedance.com/
[2] https://lore.kernel.org/linux-fsdevel/20210325082209.1067987-2-hch@lst.de

Xie Yongji (2):
  file: Export receive_fd() to modules
  binder: Use receive_fd() to receive file from another process

 drivers/android/binder.c | 4 ++--
 fs/file.c                | 6 ++++++
 include/linux/file.h     | 7 +++----
 3 files changed, 11 insertions(+), 6 deletions(-)

-- 
2.11.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] file: Export receive_fd() to modules

[Xie Yongji]

Export receive_fd() so that some modules can use
it to pass file descriptor across processes without
missing any security stuffs.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 fs/file.c            | 6 ++++++
 include/linux/file.h | 7 +++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 56986e55befa..2a80c6c3e147 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1107,6 +1107,12 @@ int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags)
 	return new_fd;
 }
 
+int receive_fd(struct file *file, unsigned int o_flags)
+{
+	return __receive_fd(file, NULL, o_flags);
+}
+EXPORT_SYMBOL(receive_fd);
+
 static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags)
 {
 	int err = -EBADF;
diff --git a/include/linux/file.h b/include/linux/file.h
index 2de2e4613d7b..51e830b4fe3a 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -94,6 +94,9 @@ extern void fd_install(unsigned int fd, struct file *file);
 
 extern int __receive_fd(struct file *file, int __user *ufd,
 			unsigned int o_flags);
+
+extern int receive_fd(struct file *file, unsigned int o_flags);
+
 static inline int receive_fd_user(struct file *file, int __user *ufd,
 				  unsigned int o_flags)
 {
@@ -101,10 +104,6 @@ static inline int receive_fd_user(struct file *file, int __user *ufd,
 		return -EFAULT;
 	return __receive_fd(file, ufd, o_flags);
 }
-static inline int receive_fd(struct file *file, unsigned int o_flags)
-{
-	return __receive_fd(file, NULL, o_flags);
-}
 int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags);
 
 extern void flush_delayed_fput(void);
-- 
2.11.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Xie Yongji]

Use receive_fd() to receive file from another process instead of
combination of get_unused_fd_flags() and fd_install(). This simplifies
the logic and also makes sure we don't miss any security stuff.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/android/binder.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c119736ca56a..080bcab7d632 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3728,7 +3728,7 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
 	int ret = 0;
 
 	list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
-		int fd = get_unused_fd_flags(O_CLOEXEC);
+		int fd  = receive_fd(fixup->file, O_CLOEXEC);
 
 		if (fd < 0) {
 			binder_debug(BINDER_DEBUG_TRANSACTION,
@@ -3741,7 +3741,7 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
 			     "fd fixup txn %d fd %d\n",
 			     t->debug_id, fd);
 		trace_binder_transaction_fd_recv(t, fd, fixup->offset);
-		fd_install(fd, fixup->file);
+		fput(fixup->file);
 		fixup->file = NULL;
 		if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
 						fixup->offset, &fd,
-- 
2.11.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] file: Export receive_fd() to modules

[Greg KH]

On Thu, Apr 01, 2021 at 05:09:31PM +0800, Xie Yongji wrote:
> Export receive_fd() so that some modules can use
> it to pass file descriptor across processes without
> missing any security stuffs.
> 
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  fs/file.c            | 6 ++++++
>  include/linux/file.h | 7 +++----
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 56986e55befa..2a80c6c3e147 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1107,6 +1107,12 @@ int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags)
>  	return new_fd;
>  }
>  
> +int receive_fd(struct file *file, unsigned int o_flags)
> +{
> +	return __receive_fd(file, NULL, o_flags);
> +}
> +EXPORT_SYMBOL(receive_fd);

What module uses this?

And why not EXPORT_SYMBOL_GPL()?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/2] Export receive_fd() to modules and do some cleanups

[Greg KH]

On Thu, Apr 01, 2021 at 05:09:30PM +0800, Xie Yongji wrote:
> This series starts from Christian's comments on the series[1].
> We'd like to export receive_fd() which can not only be used by
> our module in the series[1] but also allow further cleanups
> like patch 2 does.

But binder can not be a module, so why do you need this?

confused,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Greg KH]

On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> Use receive_fd() to receive file from another process instead of
> combination of get_unused_fd_flags() and fd_install(). This simplifies
> the logic and also makes sure we don't miss any security stuff.

But no logic is simplified here, and nothing is "missed", so I do not
understand this change at all.

> 
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  drivers/android/binder.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index c119736ca56a..080bcab7d632 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3728,7 +3728,7 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
>  	int ret = 0;
>  
>  	list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
> -		int fd = get_unused_fd_flags(O_CLOEXEC);
> +		int fd  = receive_fd(fixup->file, O_CLOEXEC);

Why 2 spaces?

>  
>  		if (fd < 0) {
>  			binder_debug(BINDER_DEBUG_TRANSACTION,
> @@ -3741,7 +3741,7 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
>  			     "fd fixup txn %d fd %d\n",
>  			     t->debug_id, fd);
>  		trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> -		fd_install(fd, fixup->file);
> +		fput(fixup->file);

Are you sure this is the same???

I d onot understand the need for this change at all, what is wrong with
the existing code here?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: [PATCH 0/2] Export receive_fd() to modules and do some cleanups

[Yongji Xie]

On Thu, Apr 1, 2021 at 5:53 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Apr 01, 2021 at 05:09:30PM +0800, Xie Yongji wrote:
> > This series starts from Christian's comments on the series[1].
> > We'd like to export receive_fd() which can not only be used by
> > our module in the series[1] but also allow further cleanups
> > like patch 2 does.
>
> But binder can not be a module, so why do you need this?
>

Oh, right. I miss it.

Thanks,
Yongji
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Yongji Xie]

On Thu, Apr 1, 2021 at 5:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> > Use receive_fd() to receive file from another process instead of
> > combination of get_unused_fd_flags() and fd_install(). This simplifies
> > the logic and also makes sure we don't miss any security stuff.
>
> But no logic is simplified here, and nothing is "missed", so I do not
> understand this change at all.
>

I noticed that we have security_binder_transfer_file() when we
transfer some fds. I'm not sure whether we need something like
security_file_receive() here?

Thanks,
Yongji
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/2] Export receive_fd() to modules and do some cleanups

[Christian Brauner]

On Thu, Apr 01, 2021 at 05:09:30PM +0800, Xie Yongji wrote:
> This series starts from Christian's comments on the series[1].
> We'd like to export receive_fd() which can not only be used by
> our module in the series[1] but also allow further cleanups
> like patch 2 does.
> 
> Now this series is based on Christoph's patch[2].
> 
> [1] https://lore.kernel.org/linux-fsdevel/20210331080519.172-1-xieyongji@bytedance.com/
> [2] https://lore.kernel.org/linux-fsdevel/20210325082209.1067987-2-hch@lst.de
> 
> Xie Yongji (2):
>   file: Export receive_fd() to modules
>   binder: Use receive_fd() to receive file from another process
> 
>  drivers/android/binder.c | 4 ++--
>  fs/file.c                | 6 ++++++
>  include/linux/file.h     | 7 +++----

Hm, I think we're still talking a bit past each other.
I'll try to illustrate what I mean in a patch series soon.

Christian
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: [PATCH 1/2] file: Export receive_fd() to modules

[Yongji Xie]

On Thu, Apr 1, 2021 at 5:52 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Apr 01, 2021 at 05:09:31PM +0800, Xie Yongji wrote:
> > Export receive_fd() so that some modules can use
> > it to pass file descriptor across processes without
> > missing any security stuffs.
> >
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> >  fs/file.c            | 6 ++++++
> >  include/linux/file.h | 7 +++----
> >  2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/file.c b/fs/file.c
> > index 56986e55befa..2a80c6c3e147 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -1107,6 +1107,12 @@ int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags)
> >       return new_fd;
> >  }
> >
> > +int receive_fd(struct file *file, unsigned int o_flags)
> > +{
> > +     return __receive_fd(file, NULL, o_flags);
> > +}
> > +EXPORT_SYMBOL(receive_fd);
>
> What module uses this?
>

Looks like now it will be only used by the module in my proposal:

https://lore.kernel.org/linux-fsdevel/20210331080519.172-1-xieyongji@bytedance.com/

> And why not EXPORT_SYMBOL_GPL()?
>

My fault, sorry.

Thanks,
Yongji
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Christian Brauner]

On Thu, Apr 01, 2021 at 11:54:45AM +0200, Greg KH wrote:
> On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> > Use receive_fd() to receive file from another process instead of
> > combination of get_unused_fd_flags() and fd_install(). This simplifies
> > the logic and also makes sure we don't miss any security stuff.
> 
> But no logic is simplified here, and nothing is "missed", so I do not
> understand this change at all.
> 
> > 
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> >  drivers/android/binder.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index c119736ca56a..080bcab7d632 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -3728,7 +3728,7 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
> >  	int ret = 0;
> >  
> >  	list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
> > -		int fd = get_unused_fd_flags(O_CLOEXEC);
> > +		int fd  = receive_fd(fixup->file, O_CLOEXEC);
> 
> Why 2 spaces?
> 
> >  
> >  		if (fd < 0) {
> >  			binder_debug(BINDER_DEBUG_TRANSACTION,
> > @@ -3741,7 +3741,7 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
> >  			     "fd fixup txn %d fd %d\n",
> >  			     t->debug_id, fd);
> >  		trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> > -		fd_install(fd, fixup->file);
> > +		fput(fixup->file);
> 
> Are you sure this is the same???
> 
> I d onot understand the need for this change at all, what is wrong with
> the existing code here?

I suggested something like this.
Some time back we added receive_fd() for seccomp and SCM_RIGHTS to have
a unified way of installing file descriptors including taking care of
handling sockets and running security hooks. The helper also encompasses
the whole get_unused_fd() + fd_install dance.
My suggestion was to look at all the places were we currently open-code
this in drivers/:

drivers/android/binder.c:               int fd = get_unused_fd_flags(O_CLOEXEC);
drivers/char/tpm/tpm_vtpm_proxy.c:      fd = get_unused_fd_flags(O_RDWR);
drivers/dma-buf/dma-buf.c:      fd = get_unused_fd_flags(flags);
drivers/dma-buf/sw_sync.c:      int fd = get_unused_fd_flags(O_CLOEXEC);
drivers/dma-buf/sync_file.c:    int fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpio/gpiolib-cdev.c:    fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
drivers/gpio/gpiolib-cdev.c:    fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
drivers/gpio/gpiolib-cdev.c:    fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:         fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/drm_atomic_uapi.c:      fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/drm_lease.c:    fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK));
drivers/gpu/drm/drm_syncobj.c:  fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/drm_syncobj.c:  int fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c:           out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:         out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/msm/msm_gem_submit.c:           out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/virtio/virtgpu_ioctl.c:         out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:                out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
drivers/infiniband/core/rdma_core.c:    new_fd = get_unused_fd_flags(O_CLOEXEC);
drivers/media/mc/mc-request.c:  fd = get_unused_fd_flags(O_CLOEXEC);
drivers/misc/cxl/api.c: rc = get_unused_fd_flags(flags);
drivers/scsi/cxlflash/ocxl_hw.c:        rc = get_unused_fd_flags(flags);
drivers/scsi/cxlflash/ocxl_hw.c:                dev_err(dev, "%s: get_unused_fd_flags failed rc=%d\n",
drivers/tty/pty.c:      fd = get_unused_fd_flags(flags);
drivers/vfio/vfio.c:    ret = get_unused_fd_flags(O_CLOEXEC);
drivers/virt/nitro_enclaves/ne_misc_dev.c:      enclave_fd = get_unused_fd_flags(O_CLOEXEC);

and see whether all of them can be switched to simply using
receive_fd(). I did a completely untested rough sketch to illustrate
what I meant by using binder and devpts Xie seems to have just picked
those two. But the change is obviously only worth it if all or nearly
all callers can be switched over without risk of regression.
It would most likely simplify quite a few codepaths though especially in
the error paths since we can get rid of put_unused_fd() cleanup.

But it requires buy in from others obviously.

Christian
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Greg KH]

On Thu, Apr 01, 2021 at 06:12:51PM +0800, Yongji Xie wrote:
> On Thu, Apr 1, 2021 at 5:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> > > Use receive_fd() to receive file from another process instead of
> > > combination of get_unused_fd_flags() and fd_install(). This simplifies
> > > the logic and also makes sure we don't miss any security stuff.
> >
> > But no logic is simplified here, and nothing is "missed", so I do not
> > understand this change at all.
> >
> 
> I noticed that we have security_binder_transfer_file() when we
> transfer some fds. I'm not sure whether we need something like
> security_file_receive() here?

Why would you?  And where is "here"?

still confused,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Yongji Xie]

On Thu, Apr 1, 2021 at 6:40 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Thu, Apr 01, 2021 at 11:54:45AM +0200, Greg KH wrote:
> > On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> > > Use receive_fd() to receive file from another process instead of
> > > combination of get_unused_fd_flags() and fd_install(). This simplifies
> > > the logic and also makes sure we don't miss any security stuff.
> >
> > But no logic is simplified here, and nothing is "missed", so I do not
> > understand this change at all.
> >
> > >
> > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > ---
> > >  drivers/android/binder.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index c119736ca56a..080bcab7d632 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -3728,7 +3728,7 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
> > >     int ret = 0;
> > >
> > >     list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
> > > -           int fd = get_unused_fd_flags(O_CLOEXEC);
> > > +           int fd  = receive_fd(fixup->file, O_CLOEXEC);
> >
> > Why 2 spaces?
> >
> > >
> > >             if (fd < 0) {
> > >                     binder_debug(BINDER_DEBUG_TRANSACTION,
> > > @@ -3741,7 +3741,7 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
> > >                          "fd fixup txn %d fd %d\n",
> > >                          t->debug_id, fd);
> > >             trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> > > -           fd_install(fd, fixup->file);
> > > +           fput(fixup->file);
> >
> > Are you sure this is the same???
> >
> > I d onot understand the need for this change at all, what is wrong with
> > the existing code here?
>
> I suggested something like this.
> Some time back we added receive_fd() for seccomp and SCM_RIGHTS to have
> a unified way of installing file descriptors including taking care of
> handling sockets and running security hooks. The helper also encompasses
> the whole get_unused_fd() + fd_install dance.
> My suggestion was to look at all the places were we currently open-code
> this in drivers/:
>

Sorry for not following your suggestion. Yes, I looked at those
places. But I found that we will add security_file_receive()
implicitly if we replace get_unused_fd() + fd_install() with
receive_fd() for most drivers. Not sure if there will be some
regression. So I only do that where I think security_file_receive() is
needed, that is this patch. Although it looks like this is not a good
idea now...

Thanks,
Yongji
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Yongji Xie]

On Thu, Apr 1, 2021 at 6:42 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Apr 01, 2021 at 06:12:51PM +0800, Yongji Xie wrote:
> > On Thu, Apr 1, 2021 at 5:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> > > > Use receive_fd() to receive file from another process instead of
> > > > combination of get_unused_fd_flags() and fd_install(). This simplifies
> > > > the logic and also makes sure we don't miss any security stuff.
> > >
> > > But no logic is simplified here, and nothing is "missed", so I do not
> > > understand this change at all.
> > >
> >
> > I noticed that we have security_binder_transfer_file() when we
> > transfer some fds. I'm not sure whether we need something like
> > security_file_receive() here?
>
> Why would you?  And where is "here"?
>
> still confused,
>

I mean do we need to go through the file_receive seccomp notifier when
we receive fd (use get_unused_fd_flags() + fd_install now) from
another process in binder_apply_fd_fixups().

Thanks,
Yongji
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Greg KH]

On Thu, Apr 01, 2021 at 07:29:45PM +0800, Yongji Xie wrote:
> On Thu, Apr 1, 2021 at 6:42 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Apr 01, 2021 at 06:12:51PM +0800, Yongji Xie wrote:
> > > On Thu, Apr 1, 2021 at 5:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> > > > > Use receive_fd() to receive file from another process instead of
> > > > > combination of get_unused_fd_flags() and fd_install(). This simplifies
> > > > > the logic and also makes sure we don't miss any security stuff.
> > > >
> > > > But no logic is simplified here, and nothing is "missed", so I do not
> > > > understand this change at all.
> > > >
> > >
> > > I noticed that we have security_binder_transfer_file() when we
> > > transfer some fds. I'm not sure whether we need something like
> > > security_file_receive() here?
> >
> > Why would you?  And where is "here"?
> >
> > still confused,
> >
> 
> I mean do we need to go through the file_receive seccomp notifier when
> we receive fd (use get_unused_fd_flags() + fd_install now) from
> another process in binder_apply_fd_fixups().

Why?  this is internal things, why does seccomp come into play here?

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: Re: Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Yongji Xie]

On Thu, Apr 1, 2021 at 7:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Apr 01, 2021 at 07:29:45PM +0800, Yongji Xie wrote:
> > On Thu, Apr 1, 2021 at 6:42 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Apr 01, 2021 at 06:12:51PM +0800, Yongji Xie wrote:
> > > > On Thu, Apr 1, 2021 at 5:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> > > > > > Use receive_fd() to receive file from another process instead of
> > > > > > combination of get_unused_fd_flags() and fd_install(). This simplifies
> > > > > > the logic and also makes sure we don't miss any security stuff.
> > > > >
> > > > > But no logic is simplified here, and nothing is "missed", so I do not
> > > > > understand this change at all.
> > > > >
> > > >
> > > > I noticed that we have security_binder_transfer_file() when we
> > > > transfer some fds. I'm not sure whether we need something like
> > > > security_file_receive() here?
> > >
> > > Why would you?  And where is "here"?
> > >
> > > still confused,
> > >
> >
> > I mean do we need to go through the file_receive seccomp notifier when
> > we receive fd (use get_unused_fd_flags() + fd_install now) from
> > another process in binder_apply_fd_fixups().
>
> Why?  this is internal things, why does seccomp come into play here?
>

We already have security_binder_transfer_file() to control the sender
process. So for the receiver process, do we need the seccomp too? Or
do I miss something here?

Thanks,
Yongji
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: Re: Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Greg KH]

On Thu, Apr 01, 2021 at 08:28:02PM +0800, Yongji Xie wrote:
> On Thu, Apr 1, 2021 at 7:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Apr 01, 2021 at 07:29:45PM +0800, Yongji Xie wrote:
> > > On Thu, Apr 1, 2021 at 6:42 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Apr 01, 2021 at 06:12:51PM +0800, Yongji Xie wrote:
> > > > > On Thu, Apr 1, 2021 at 5:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> > > > > > > Use receive_fd() to receive file from another process instead of
> > > > > > > combination of get_unused_fd_flags() and fd_install(). This simplifies
> > > > > > > the logic and also makes sure we don't miss any security stuff.
> > > > > >
> > > > > > But no logic is simplified here, and nothing is "missed", so I do not
> > > > > > understand this change at all.
> > > > > >
> > > > >
> > > > > I noticed that we have security_binder_transfer_file() when we
> > > > > transfer some fds. I'm not sure whether we need something like
> > > > > security_file_receive() here?
> > > >
> > > > Why would you?  And where is "here"?
> > > >
> > > > still confused,
> > > >
> > >
> > > I mean do we need to go through the file_receive seccomp notifier when
> > > we receive fd (use get_unused_fd_flags() + fd_install now) from
> > > another process in binder_apply_fd_fixups().
> >
> > Why?  this is internal things, why does seccomp come into play here?
> >
> 
> We already have security_binder_transfer_file() to control the sender
> process. So for the receiver process, do we need the seccomp too? Or
> do I miss something here?

I do not know, is this something that is a requirement that seccomp
handle all filesystem handles sent to a process?  I do not know the
seccomp "guarantee" that well, sorry.

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: Re: Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Kees Cook]

On Thu, Apr 01, 2021 at 04:09:57PM +0200, Greg KH wrote:
> On Thu, Apr 01, 2021 at 08:28:02PM +0800, Yongji Xie wrote:
> > On Thu, Apr 1, 2021 at 7:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Apr 01, 2021 at 07:29:45PM +0800, Yongji Xie wrote:
> > > > On Thu, Apr 1, 2021 at 6:42 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Thu, Apr 01, 2021 at 06:12:51PM +0800, Yongji Xie wrote:
> > > > > > On Thu, Apr 1, 2021 at 5:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> > > > > > > > Use receive_fd() to receive file from another process instead of
> > > > > > > > combination of get_unused_fd_flags() and fd_install(). This simplifies
> > > > > > > > the logic and also makes sure we don't miss any security stuff.
> > > > > > >
> > > > > > > But no logic is simplified here, and nothing is "missed", so I do not
> > > > > > > understand this change at all.
> > > > > > >
> > > > > >
> > > > > > I noticed that we have security_binder_transfer_file() when we
> > > > > > transfer some fds. I'm not sure whether we need something like
> > > > > > security_file_receive() here?
> > > > >
> > > > > Why would you?  And where is "here"?
> > > > >
> > > > > still confused,
> > > > >
> > > >
> > > > I mean do we need to go through the file_receive seccomp notifier when
> > > > we receive fd (use get_unused_fd_flags() + fd_install now) from
> > > > another process in binder_apply_fd_fixups().
> > >
> > > Why?  this is internal things, why does seccomp come into play here?
> > >
> > 
> > We already have security_binder_transfer_file() to control the sender
> > process. So for the receiver process, do we need the seccomp too? Or
> > do I miss something here?
> 
> I do not know, is this something that is a requirement that seccomp
> handle all filesystem handles sent to a process?  I do not know the
> seccomp "guarantee" that well, sorry.

This is an extremely confused thread. seccomp _uses_ the receive_fd()
API. receive_fd() calls the security_file_receive() LSM hook. The
security_binder_*() LSM hooks are different yet.

Please, let's wait for Christian to clarify his idea first.

-- 
Kees Cook
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Al Viro]

On Thu, Apr 01, 2021 at 12:40:34PM +0200, Christian Brauner wrote:
> My suggestion was to look at all the places were we currently open-code
> this in drivers/:
> 
> drivers/android/binder.c:               int fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/char/tpm/tpm_vtpm_proxy.c:      fd = get_unused_fd_flags(O_RDWR);
> drivers/dma-buf/dma-buf.c:      fd = get_unused_fd_flags(flags);
> drivers/dma-buf/sw_sync.c:      int fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/dma-buf/sync_file.c:    int fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpio/gpiolib-cdev.c:    fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> drivers/gpio/gpiolib-cdev.c:    fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> drivers/gpio/gpiolib-cdev.c:    fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:         fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/drm_atomic_uapi.c:      fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/drm_lease.c:    fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK));
> drivers/gpu/drm/drm_syncobj.c:  fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/drm_syncobj.c:  int fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c:           out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:         out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/msm/msm_gem_submit.c:           out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/virtio/virtgpu_ioctl.c:         out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:                out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/infiniband/core/rdma_core.c:    new_fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/media/mc/mc-request.c:  fd = get_unused_fd_flags(O_CLOEXEC);
> drivers/misc/cxl/api.c: rc = get_unused_fd_flags(flags);
> drivers/scsi/cxlflash/ocxl_hw.c:        rc = get_unused_fd_flags(flags);
> drivers/scsi/cxlflash/ocxl_hw.c:                dev_err(dev, "%s: get_unused_fd_flags failed rc=%d\n",
> drivers/tty/pty.c:      fd = get_unused_fd_flags(flags);
> drivers/vfio/vfio.c:    ret = get_unused_fd_flags(O_CLOEXEC);
> drivers/virt/nitro_enclaves/ne_misc_dev.c:      enclave_fd = get_unused_fd_flags(O_CLOEXEC);
> 
> and see whether all of them can be switched to simply using
> receive_fd(). I did a completely untested rough sketch to illustrate
> what I meant by using binder and devpts Xie seems to have just picked
> those two. But the change is obviously only worth it if all or nearly
> all callers can be switched over without risk of regression.
> It would most likely simplify quite a few codepaths though especially in
> the error paths since we can get rid of put_unused_fd() cleanup.
> 
> But it requires buy in from others obviously.

Opening a file can have non-trivial side effects; reserving a descriptor
can't.  Moreover, if you look at the second hit in your list, you'll see
that we do *NOT* want that combined thing there - fd_install() is
completely irreversible, so we can't do that until we made sure the
reply (struct vtpm_proxy_new_dev) has been successfully copied to
userland.  No, your receive_fd_user() does not cover that.

There's a bunch of other cases like that - the next ones on your list
are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc.

So I would consider receive_fd() as an attractive nuisance if it
is ever suggested (and available) for wide use...
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Al Viro]

On Fri, Apr 16, 2021 at 05:19:50AM +0000, Al Viro wrote:
> On Thu, Apr 01, 2021 at 12:40:34PM +0200, Christian Brauner wrote:

> > and see whether all of them can be switched to simply using
> > receive_fd(). I did a completely untested rough sketch to illustrate
> > what I meant by using binder and devpts Xie seems to have just picked
> > those two. But the change is obviously only worth it if all or nearly
> > all callers can be switched over without risk of regression.
> > It would most likely simplify quite a few codepaths though especially in
> > the error paths since we can get rid of put_unused_fd() cleanup.
> > 
> > But it requires buy in from others obviously.
> 
> Opening a file can have non-trivial side effects; reserving a descriptor
> can't.  Moreover, if you look at the second hit in your list, you'll see
> that we do *NOT* want that combined thing there - fd_install() is
> completely irreversible, so we can't do that until we made sure the
> reply (struct vtpm_proxy_new_dev) has been successfully copied to
> userland.  No, your receive_fd_user() does not cover that.
> 
> There's a bunch of other cases like that - the next ones on your list
> are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc.

FWIW, pretty much all ioctls that return descriptor as part of a structure
stored to user-supplied address tend to be that way; some don't have any
other output fields (in which case they probably would've been better off
with just passing the descriptor as return value of ioctl(2)).  Those
might be served by that receive_fd_user() helper; anything that has several
outputs won't be.  The same goes for anything that has hard-to-undo
operations as part of what they need to do:
	reserve fd
	set file up
	do hard-to-undo stuff
	install into descriptor table
is the only feasible order of operations - reservation can fail, so
it must go before the hard-to-undo part and install into descriptor
table can't be undone at all, so it must come last.  Looks like
e.g. drivers/virt/nitro_enclaves/ne_misc_dev.c case might be of
that sort...
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Christian Brauner]

On Fri, Apr 16, 2021 at 05:55:16AM +0000, Al Viro wrote:
> On Fri, Apr 16, 2021 at 05:19:50AM +0000, Al Viro wrote:
> > On Thu, Apr 01, 2021 at 12:40:34PM +0200, Christian Brauner wrote:
> 
> > > and see whether all of them can be switched to simply using
> > > receive_fd(). I did a completely untested rough sketch to illustrate
> > > what I meant by using binder and devpts Xie seems to have just picked
> > > those two. But the change is obviously only worth it if all or nearly
> > > all callers can be switched over without risk of regression.
> > > It would most likely simplify quite a few codepaths though especially in
> > > the error paths since we can get rid of put_unused_fd() cleanup.
> > > 
> > > But it requires buy in from others obviously.
> > 
> > Opening a file can have non-trivial side effects; reserving a descriptor
> > can't.  Moreover, if you look at the second hit in your list, you'll see
> > that we do *NOT* want that combined thing there - fd_install() is
> > completely irreversible, so we can't do that until we made sure the
> > reply (struct vtpm_proxy_new_dev) has been successfully copied to
> > userland.  No, your receive_fd_user() does not cover that.
> > 
> > There's a bunch of other cases like that - the next ones on your list
> > are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc.
> 
> FWIW, pretty much all ioctls that return descriptor as part of a structure
> stored to user-supplied address tend to be that way; some don't have any
> other output fields (in which case they probably would've been better off
> with just passing the descriptor as return value of ioctl(2)).  Those
> might be served by that receive_fd_user() helper; anything that has several
> outputs won't be.  The same goes for anything that has hard-to-undo
> operations as part of what they need to do:
> 	reserve fd
> 	set file up
> 	do hard-to-undo stuff
> 	install into descriptor table
> is the only feasible order of operations - reservation can fail, so
> it must go before the hard-to-undo part and install into descriptor
> table can't be undone at all, so it must come last.  Looks like
> e.g. drivers/virt/nitro_enclaves/ne_misc_dev.c case might be of
> that sort...

If receive_fd() or your receive_fd_user() proposal can replace a chunk
of open-coded places in modules where the split between reserving the
file descriptor and installing it is pointless it's probably already
worth it. Random example from io_uring where the file is already opened
way before (which yes, isn't a module afaik but another place where we
have that pattern):

static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file)
{
	int ret, fd;

	fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
	if (fd < 0)
		return fd;

	ret = io_uring_add_task_file(ctx);
	if (ret) {
		put_unused_fd(fd);
		return ret;
	}
	fd_install(fd, file);
	return fd;
}

A practical question also is whether the security receive hook thing
actually belongs into the receive_fd() and receive_fd_user() helpers for
the general case or whether that's just something that very few callers
would want. But in any case I'm unlikely to have time on my hands to
play with sm like this any time soon.

Christian
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Al Viro]

On Fri, Apr 16, 2021 at 03:42:52PM +0200, Christian Brauner wrote:
> > > are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc.
> > 
> > FWIW, pretty much all ioctls that return descriptor as part of a structure
> > stored to user-supplied address tend to be that way; some don't have any
> > other output fields (in which case they probably would've been better off
> > with just passing the descriptor as return value of ioctl(2)).  Those
> > might be served by that receive_fd_user() helper; anything that has several
> > outputs won't be.  The same goes for anything that has hard-to-undo
> > operations as part of what they need to do:
> > 	reserve fd
> > 	set file up
> > 	do hard-to-undo stuff
> > 	install into descriptor table
> > is the only feasible order of operations - reservation can fail, so
> > it must go before the hard-to-undo part and install into descriptor
> > table can't be undone at all, so it must come last.  Looks like
> > e.g. drivers/virt/nitro_enclaves/ne_misc_dev.c case might be of
> > that sort...
> 
> If receive_fd() or your receive_fd_user() proposal can replace a chunk

My what proposal?  The thing is currently in linux/file.h, put there
by Kees half a year ago...

> of open-coded places in modules where the split between reserving the
> file descriptor and installing it is pointless it's probably already
> worth it.

A helper for use in some of the simplest cases, with big fat warnings
not to touch if the things are not entirely trivial - sure, why not,
same as we have anon_inode_getfd().  But that's a convenience helper,
not a general purpose primitive.

> Random example from io_uring where the file is already opened
> way before (which yes, isn't a module afaik but another place where we
> have that pattern):
> 
> static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file)
> {
> 	int ret, fd;
> 
> 	fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
> 	if (fd < 0)
> 		return fd;
> 
> 	ret = io_uring_add_task_file(ctx);

Huh?  It's
        ret = io_uring_add_task_file(ctx, file);
in the mainline and I don't see how that sucker could work without having
file passed to it.

> 	if (ret) {
> 		put_unused_fd(fd);
> 		return ret;
> 	}
> 	fd_install(fd, file);
> 	return fd;
> }

... and that's precisely the situation where we have something that is
not obvious how to undo; look into io_uring_add_task_file()...

We have three things to do: (1) reserve a descriptor, (2) io_uring_add_task_file(),
(3) install the file.  (1) and (2) may fail, (1) is trivial to undo, (2) might be
not, (3) is impossible to undo.  So I'd say that in this particular case
io_uring is being perfectly reasonable...
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Christian Brauner]

On Fri, Apr 16, 2021 at 02:09:35PM +0000, Al Viro wrote:
> On Fri, Apr 16, 2021 at 03:42:52PM +0200, Christian Brauner wrote:
> > > > are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc.
> > > 
> > > FWIW, pretty much all ioctls that return descriptor as part of a structure
> > > stored to user-supplied address tend to be that way; some don't have any
> > > other output fields (in which case they probably would've been better off
> > > with just passing the descriptor as return value of ioctl(2)).  Those
> > > might be served by that receive_fd_user() helper; anything that has several
> > > outputs won't be.  The same goes for anything that has hard-to-undo
> > > operations as part of what they need to do:
> > > 	reserve fd
> > > 	set file up
> > > 	do hard-to-undo stuff
> > > 	install into descriptor table
> > > is the only feasible order of operations - reservation can fail, so
> > > it must go before the hard-to-undo part and install into descriptor
> > > table can't be undone at all, so it must come last.  Looks like
> > > e.g. drivers/virt/nitro_enclaves/ne_misc_dev.c case might be of
> > > that sort...
> > 
> > If receive_fd() or your receive_fd_user() proposal can replace a chunk
> 
> My what proposal?  The thing is currently in linux/file.h, put there
> by Kees half a year ago...

Yeah, I know. I was just referring to that line:

> > > might be served by that receive_fd_user() helper; anything that has several

I wasn't trying to imply you were the author or instigator of the api.

> 
> > of open-coded places in modules where the split between reserving the
> > file descriptor and installing it is pointless it's probably already
> > worth it.
> 
> A helper for use in some of the simplest cases, with big fat warnings
> not to touch if the things are not entirely trivial - sure, why not,
> same as we have anon_inode_getfd().  But that's a convenience helper,
> not a general purpose primitive.
> 
> > Random example from io_uring where the file is already opened
> > way before (which yes, isn't a module afaik but another place where we
> > have that pattern):
> > 
> > static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file)
> > {
> > 	int ret, fd;
> > 
> > 	fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
> > 	if (fd < 0)
> > 		return fd;
> > 
> > 	ret = io_uring_add_task_file(ctx);
> 
> Huh?  It's
>         ret = io_uring_add_task_file(ctx, file);
> in the mainline and I don't see how that sucker could work without having
> file passed to it.

My point here was more that the _file_ has already been opened _before_
that call to io_uring_add_task_file(). But any potential non-trivial
side-effects of opening that file that you correctly pointed out in an
earlier mail has already happened by that time. Granted there are more
obvious examples, e.g. the binder stuff.

		int fd = get_unused_fd_flags(O_CLOEXEC);

		if (fd < 0) {
			binder_debug(BINDER_DEBUG_TRANSACTION,
				     "failed fd fixup txn %d fd %d\n",
				     t->debug_id, fd);
			ret = -ENOMEM;
			break;
		}
		binder_debug(BINDER_DEBUG_TRANSACTION,
			     "fd fixup txn %d fd %d\n",
			     t->debug_id, fd);
		trace_binder_transaction_fd_recv(t, fd, fixup->offset);
		fd_install(fd, fixup->file);
		fixup->file = NULL;
		if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
						fixup->offset, &fd,
						sizeof(u32))) {
			ret = -EINVAL;
			break;
		}
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Al Viro]

On Fri, Apr 16, 2021 at 05:13:10PM +0200, Christian Brauner wrote:

> My point here was more that the _file_ has already been opened _before_
> that call to io_uring_add_task_file(). But any potential non-trivial
> side-effects of opening that file that you correctly pointed out in an
> earlier mail has already happened by that time.

The file comes from io_uring_get_file(), the entire thing is within the
io_ring_ctx constructor and the only side effect there is ->ring_sock
creation.  And that stays until the io_ring_ctx is freed.  I'm _not_
saying I like io_uring style in general, BTW - in particular,
->ring_sock->file handling is a kludge (as is too much of interation
with AF_UNIX machinery there).  But from side effects POV we are fine
there.

> Granted there are more
> obvious examples, e.g. the binder stuff.
> 
> 		int fd = get_unused_fd_flags(O_CLOEXEC);
> 
> 		if (fd < 0) {
> 			binder_debug(BINDER_DEBUG_TRANSACTION,
> 				     "failed fd fixup txn %d fd %d\n",
> 				     t->debug_id, fd);
> 			ret = -ENOMEM;
> 			break;
> 		}
> 		binder_debug(BINDER_DEBUG_TRANSACTION,
> 			     "fd fixup txn %d fd %d\n",
> 			     t->debug_id, fd);
> 		trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> 		fd_install(fd, fixup->file);
> 		fixup->file = NULL;
> 		if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
> 						fixup->offset, &fd,
> 						sizeof(u32))) {
> 			ret = -EINVAL;
> 			break;
> 		}

... and it's actually broken, since this
        /* All copies must be 32-bit aligned and 32-bit size */
	if (!check_buffer(alloc, buffer, buffer_offset, bytes))
		return -EINVAL;
in binder_alloc_copy_to_buffer() should've been done *before*
fd_install().  If anything, it's an example of the situation when
fd_receive() would be wrong...
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Christian Brauner]

On Fri, Apr 16, 2021 at 03:35:59PM +0000, Al Viro wrote:
> On Fri, Apr 16, 2021 at 05:13:10PM +0200, Christian Brauner wrote:
> 
> > My point here was more that the _file_ has already been opened _before_
> > that call to io_uring_add_task_file(). But any potential non-trivial
> > side-effects of opening that file that you correctly pointed out in an
> > earlier mail has already happened by that time.
> 
> The file comes from io_uring_get_file(), the entire thing is within the
> io_ring_ctx constructor and the only side effect there is ->ring_sock
> creation.  And that stays until the io_ring_ctx is freed.  I'm _not_
> saying I like io_uring style in general, BTW - in particular,
> ->ring_sock->file handling is a kludge (as is too much of interation
> with AF_UNIX machinery there).  But from side effects POV we are fine
> there.
> 
> > Granted there are more
> > obvious examples, e.g. the binder stuff.
> > 
> > 		int fd = get_unused_fd_flags(O_CLOEXEC);
> > 
> > 		if (fd < 0) {
> > 			binder_debug(BINDER_DEBUG_TRANSACTION,
> > 				     "failed fd fixup txn %d fd %d\n",
> > 				     t->debug_id, fd);
> > 			ret = -ENOMEM;
> > 			break;
> > 		}
> > 		binder_debug(BINDER_DEBUG_TRANSACTION,
> > 			     "fd fixup txn %d fd %d\n",
> > 			     t->debug_id, fd);
> > 		trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> > 		fd_install(fd, fixup->file);
> > 		fixup->file = NULL;
> > 		if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
> > 						fixup->offset, &fd,
> > 						sizeof(u32))) {
> > 			ret = -EINVAL;
> > 			break;
> > 		}
> 
> ... and it's actually broken, since this
>         /* All copies must be 32-bit aligned and 32-bit size */
> 	if (!check_buffer(alloc, buffer, buffer_offset, bytes))
> 		return -EINVAL;
> in binder_alloc_copy_to_buffer() should've been done *before*
> fd_install().  If anything, it's an example of the situation when
> fd_receive() would be wrong...

They could probably refactor this but I'm not sure why they'd bother. If
they fail processing any of those files they end up aborting the
whole transaction.
(And the original code didn't check the error code btw.)
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Christian Brauner]

On Fri, Apr 16, 2021 at 05:58:25PM +0200, Christian Brauner wrote:
> On Fri, Apr 16, 2021 at 03:35:59PM +0000, Al Viro wrote:
> > On Fri, Apr 16, 2021 at 05:13:10PM +0200, Christian Brauner wrote:
> > 
> > > My point here was more that the _file_ has already been opened _before_
> > > that call to io_uring_add_task_file(). But any potential non-trivial
> > > side-effects of opening that file that you correctly pointed out in an
> > > earlier mail has already happened by that time.
> > 
> > The file comes from io_uring_get_file(), the entire thing is within the
> > io_ring_ctx constructor and the only side effect there is ->ring_sock
> > creation.  And that stays until the io_ring_ctx is freed.  I'm _not_
> > saying I like io_uring style in general, BTW - in particular,
> > ->ring_sock->file handling is a kludge (as is too much of interation
> > with AF_UNIX machinery there).  But from side effects POV we are fine
> > there.
> > 
> > > Granted there are more
> > > obvious examples, e.g. the binder stuff.
> > > 
> > > 		int fd = get_unused_fd_flags(O_CLOEXEC);
> > > 
> > > 		if (fd < 0) {
> > > 			binder_debug(BINDER_DEBUG_TRANSACTION,
> > > 				     "failed fd fixup txn %d fd %d\n",
> > > 				     t->debug_id, fd);
> > > 			ret = -ENOMEM;
> > > 			break;
> > > 		}
> > > 		binder_debug(BINDER_DEBUG_TRANSACTION,
> > > 			     "fd fixup txn %d fd %d\n",
> > > 			     t->debug_id, fd);
> > > 		trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> > > 		fd_install(fd, fixup->file);
> > > 		fixup->file = NULL;
> > > 		if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
> > > 						fixup->offset, &fd,
> > > 						sizeof(u32))) {
> > > 			ret = -EINVAL;
> > > 			break;
> > > 		}
> > 
> > ... and it's actually broken, since this
> >         /* All copies must be 32-bit aligned and 32-bit size */
> > 	if (!check_buffer(alloc, buffer, buffer_offset, bytes))
> > 		return -EINVAL;
> > in binder_alloc_copy_to_buffer() should've been done *before*
> > fd_install().  If anything, it's an example of the situation when
> > fd_receive() would be wrong...
> 
> They could probably refactor this but I'm not sure why they'd bother. If
> they fail processing any of those files they end up aborting the
> whole transaction.
> (And the original code didn't check the error code btw.)

(dma_buf_fd() seems like another good candidate. But again, I don't have
any plans to shove this down anyone's throat.)
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Al Viro]

On Fri, Apr 16, 2021 at 06:00:38PM +0200, Christian Brauner wrote:

> (dma_buf_fd() seems like another good candidate. But again, I don't have
> any plans to shove this down anyone's throat.)

Sure, there are candidates for such a helper.  Just as there are legitimate
users of anon_inode_getfd().

However, it is *NOT* something we can use as a vehicle for some hooks (pardon
the obscenity); it won't be consistently used in all cases - it simply is not
feasible for many of the fd_install() users.

Incidentally, looking at the user of receive_fd_user(), I would say that it
would be easier to follow in this form:
	case VDUSE_IOTLB_GET_ENTRY: {
		struct vduse_iotlb_entry entry;
		struct vhost_iotlb_map *map;
		struct vduse_iova_domain *domain = dev->domain;
		struct file *f = NULL;

		if (copy_from_user(&entry, argp, sizeof(entry)))
			return -EFAULT;
		entry.fd = get_unused_fd_flags(perm_to_file_flags(entry.perm));
		if (entry.fd < 0)
			return entry.fd;
		spin_lock(&domain->iotlb_lock);
		map = vhost_iotlb_itree_first(domain->iotlb,
					      entry.start, entry.start + 1);
		if (map) {
			struct vdpa_map_file *map_file = map->opaque;

			f = get_file(map_file->file);
			entry.offset = map_file->offset;
			entry.start = map->start;
			entry.last = map->last;
			entry.perm = map->perm;
		}
		spin_unlock(&domain->iotlb_lock);
		if (!f) {
			put_unused_fd(entry.fd);
			return -EINVAL;
		}
		if (copy_to_user(argp, &entry, sizeof(entry))) {
			put_unused_fd(entry.fd);
			fput(f);
			return -EFAULT;
		}
		// point of no return
		fd_install(entry.fd, f);
		return 0;
	}
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Al Viro]

On Fri, Apr 16, 2021 at 05:58:15PM +0200, Christian Brauner wrote:

> They could probably refactor this but I'm not sure why they'd bother. If
> they fail processing any of those files they end up aborting the
> whole transaction.
> (And the original code didn't check the error code btw.)

Wait a sec...  What does aborting the transaction do to descriptor table?
<rereads>
Oh, lovely...

binder_apply_fd_fixups() is deeply misguided.  What it should do is
	* go through t->fd_fixups, reserving descriptor numbers and
putting them into t->buffer (and I'd probably duplicate them into
struct binder_txn_fd_fixup).  Cleanup in case of failure: go through
the list, releasing the descriptors we'd already reserved, doing
fput() on fixup->file in all entries and freeing the entries as
we go.
	* On success, go through the list, doing fd_install() and
freeing the entries.

That's it.  No rereading from the buffer, no binder_deferred_fd_close()
crap, etc.

Again, YOU CAN NOT UNDO fd_install().  Ever.  Kernel can not decide it
shouldn't have put something in descriptor table and take it back.
You can't unring that bell.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Al Viro]

On Fri, Apr 16, 2021 at 05:30:41PM +0000, Al Viro wrote:
> On Fri, Apr 16, 2021 at 05:58:15PM +0200, Christian Brauner wrote:
> 
> > They could probably refactor this but I'm not sure why they'd bother. If
> > they fail processing any of those files they end up aborting the
> > whole transaction.
> > (And the original code didn't check the error code btw.)
> 
> Wait a sec...  What does aborting the transaction do to descriptor table?
> <rereads>
> Oh, lovely...
> 
> binder_apply_fd_fixups() is deeply misguided.  What it should do is
> 	* go through t->fd_fixups, reserving descriptor numbers and
> putting them into t->buffer (and I'd probably duplicate them into
> struct binder_txn_fd_fixup).  Cleanup in case of failure: go through
> the list, releasing the descriptors we'd already reserved, doing
> fput() on fixup->file in all entries and freeing the entries as
> we go.
> 	* On success, go through the list, doing fd_install() and
> freeing the entries.

Something like this:

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c119736ca56a..b0c5f7e625f3 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2195,6 +2195,7 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
 	fixup->offset = fd_offset;
 	trace_binder_transaction_fd_send(t, fd, fixup->offset);
 	list_add_tail(&fixup->fixup_entry, &t->fd_fixups);
+	fixup->target_fd = -1;
 
 	return ret;
 
@@ -3707,25 +3708,10 @@ static int binder_wait_for_work(struct binder_thread *thread,
 	return ret;
 }
 
-/**
- * binder_apply_fd_fixups() - finish fd translation
- * @proc:         binder_proc associated @t->buffer
- * @t:	binder transaction with list of fd fixups
- *
- * Now that we are in the context of the transaction target
- * process, we can allocate and install fds. Process the
- * list of fds to translate and fixup the buffer with the
- * new fds.
- *
- * If we fail to allocate an fd, then free the resources by
- * fput'ing files that have not been processed and ksys_close'ing
- * any fds that have already been allocated.
- */
-static int binder_apply_fd_fixups(struct binder_proc *proc,
+static int binder_reserve_fds(struct binder_proc *proc,
 				  struct binder_transaction *t)
 {
-	struct binder_txn_fd_fixup *fixup, *tmp;
-	int ret = 0;
+	struct binder_txn_fd_fixup *fixup;
 
 	list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
 		int fd = get_unused_fd_flags(O_CLOEXEC);
@@ -3734,42 +3720,55 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
 			binder_debug(BINDER_DEBUG_TRANSACTION,
 				     "failed fd fixup txn %d fd %d\n",
 				     t->debug_id, fd);
-			ret = -ENOMEM;
-			break;
+			return -ENOMEM;
 		}
 		binder_debug(BINDER_DEBUG_TRANSACTION,
 			     "fd fixup txn %d fd %d\n",
 			     t->debug_id, fd);
 		trace_binder_transaction_fd_recv(t, fd, fixup->offset);
-		fd_install(fd, fixup->file);
-		fixup->file = NULL;
+		fixup->target_fd = fd;
 		if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
 						fixup->offset, &fd,
-						sizeof(u32))) {
-			ret = -EINVAL;
-			break;
-		}
+						sizeof(u32)))
+			return -EINVAL;
 	}
-	list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
-		if (fixup->file) {
-			fput(fixup->file);
-		} else if (ret) {
-			u32 fd;
-			int err;
-
-			err = binder_alloc_copy_from_buffer(&proc->alloc, &fd,
-							    t->buffer,
-							    fixup->offset,
-							    sizeof(fd));
-			WARN_ON(err);
-			if (!err)
-				binder_deferred_fd_close(fd);
+	return 0;
+}
+
+/**
+ * binder_apply_fd_fixups() - finish fd translation
+ * @proc:         binder_proc associated @t->buffer
+ * @t:	binder transaction with list of fd fixups
+ *
+ * Now that we are in the context of the transaction target
+ * process, we can allocate fds. Process the list of fds to
+ * translate and fixup the buffer with the new fds.
+ *
+ * If we fail to allocate an fd, then free the resources by
+ * releasing fds we'd allocated.  Otherwise transfer all files
+ * from fixups to the descriptors we'd allocated for them.
+ *
+ * In either case, finish with freeing the fixups.
+ */
+static int binder_apply_fd_fixups(struct binder_proc *proc,
+				  struct binder_transaction *t)
+{
+	struct binder_txn_fd_fixup *fixup;
+	int err = binder_reserve_fds(proc, t);
+
+	if (unlikely(err)) {
+		list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
+			if (fixup->target_fd >= 0)
+				put_unused_fd(fixup->target_fd);
+		}
+	} else {
+		list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
+			fd_install(fixup->target_fd, fixup->file);
+			fixup->file = NULL;
 		}
-		list_del(&fixup->fixup_entry);
-		kfree(fixup);
 	}
-
-	return ret;
+	binder_free_txn_fixups(t);
+	return err;
 }
 
 static int binder_thread_read(struct binder_proc *proc,
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 6cd79011e35d..16ffc5f748ce 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -497,6 +497,7 @@ struct binder_txn_fd_fixup {
 	struct list_head fixup_entry;
 	struct file *file;
 	size_t offset;
+	int target_fd;
 };
 
 struct binder_transaction {
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel