Re: [PATCH] libselinux: Always close status page fd

[Petr Lautrbach <plautrba@redhat.com>]

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.