diff mbox series

libselinux: Always close status page fd

Message ID 20210114133910.282686-1-plautrba@redhat.com (mailing list archive)
State Superseded
Headers show
Series libselinux: Always close status page fd | expand

Commit Message

Petr Lautrbach Jan. 14, 2021, 1:39 p.m. UTC
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@redhat.com>
---
 libselinux/src/sestatus.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

William Roberts Jan. 14, 2021, 2:30 p.m. UTC | #1
On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@redhat.com> 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@redhat.com>
> ---
>  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.
William Roberts Jan. 14, 2021, 2:33 p.m. UTC | #2
On Thu, Jan 14, 2021 at 8:30 AM William Roberts
<bill.c.roberts@gmail.com> wrote:
>
> On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@redhat.com> 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@redhat.com>
> > ---
> >  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.
William Roberts Jan. 14, 2021, 2:46 p.m. UTC | #3
On Thu, Jan 14, 2021 at 8:33 AM William Roberts
<bill.c.roberts@gmail.com> wrote:
>
> On Thu, Jan 14, 2021 at 8:30 AM William Roberts
> <bill.c.roberts@gmail.com> wrote:
> >
> > On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@redhat.com> 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@redhat.com>
> > > ---
> > >  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.
Petr Lautrbach Jan. 14, 2021, 2:54 p.m. UTC | #4
William Roberts <bill.c.roberts@gmail.com> writes:

> On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@redhat.com> 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@redhat.com>
>> ---
>>  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;
>>         }

I'll drop this one. It's already closed by avc_netlink_close()

>
>> @@ -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);

I believe this is correct. selinux_stats_fd is not assigned when mmap()
doesn't return MAP_FAILED

>>  }
>> --
>> 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.
William Roberts Jan. 14, 2021, 3:11 p.m. UTC | #5
On Thu, Jan 14, 2021 at 8:54 AM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> William Roberts <bill.c.roberts@gmail.com> writes:
>
> > On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@redhat.com> 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@redhat.com>
> >> ---
> >>  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;
> >>         }
>
> I'll drop this one. It's already closed by avc_netlink_close()

We can actually get rid of selinux_status_fd all together. The call to
avc_netlink_acquire_fd() just returns it's static cache of the fd acquired
in avc_netlink_open(). So we can just pair the avc_netlink_open with
it's avc_netlink_code().

>
> >
> >> @@ -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);
>
> I believe this is correct. selinux_stats_fd is not assigned when mmap()
> doesn't return MAP_FAILED
>
> >>  }
> >> --
> >> 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.
>
diff mbox series

Patch

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);
 }