On Thu, Jan 14, 2021 at 8:33 AM William Roberts
<bill.c.roberts@xxxxxxxxx> wrote:
>
> On Thu, Jan 14, 2021 at 8:30 AM William Roberts
> <bill.c.roberts@xxxxxxxxx> wrote:
> >
> > On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@xxxxxxxxxx> wrote:
> > >
> > > According to mmap(2) after the mmap() call has returned, the file
> > > descriptor, fd, can be closed immediately without invalidating the
> > > mapping.
> > >
> > > Signed-off-by: Petr Lautrbach <plautrba@xxxxxxxxxx>
> > > ---
> > > libselinux/src/sestatus.c | 6 ++----
> > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
> > > index 9ff2785d876a..6a243b7bcdfb 100644
> > > --- a/libselinux/src/sestatus.c
> > > +++ b/libselinux/src/sestatus.c
> > > @@ -298,11 +298,10 @@ int selinux_status_open(int fallback)
> > > goto error;
> > >
> > > selinux_status = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd, 0);
> > > + close(fd);
> > > if (selinux_status == MAP_FAILED) {
> > > - close(fd);
> > > goto error;
> > > }
> > > - selinux_status_fd = fd;
> > > last_seqno = (uint32_t)(-1);
> > >
> > > /* sequence must not be changed during references */
> > > @@ -379,6 +378,7 @@ void selinux_status_close(void)
> > > avc_netlink_release_fd();
> > > avc_netlink_close();
> > > selinux_status = NULL;
> > > + close(selinux_status_fd);
> > > return;
> > > }
> > >
> > > @@ -388,7 +388,5 @@ void selinux_status_close(void)
> > > munmap(selinux_status, pagesize);
> > > selinux_status = NULL;
> > >
> > > - close(selinux_status_fd);
> > > - selinux_status_fd = -1;
> > > last_seqno = (uint32_t)(-1);
> > > }
> > > --
> > > 2.30.0
> > >
> >
> > Nack, the fd in the mmap of the status page and the selinux_status_fd
> > (avc mount) are different fd's.
> > The selinux_status_fd is for the AVC netlink socket fallback. If you
> > drop those hunks I'd take the patch.
>
> Sorry I misread that, I missed the assignment from fd to that variable...
>
> Ack, this should fix that umount issue hopefully I didn't test that though.
As an aside though, I did notice a bug in the existing code:
A failure in:
selinux_status_fd = avc_netlink_acquire_fd();
Will cause the code to still return success to the caller of
selinux_status_open().
And cause the close to return EBADF which is silently ignored and
doesn't matter.
