[v2] tpm: fix crash when FD >= 1024

[PATCH v2] tpm: fix crash when FD >= 1024

Posted by marcandre.lureau@redhat.com 7 months, 2 weeks ago

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Replace select() with poll() to fix a crash when QEMU has a large number
of FDs.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2020133

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 backends/tpm/tpm_util.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index a6e6d3e72f..1856589c3b 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -112,12 +112,8 @@ static int tpm_util_request(int fd,
                             void *response,
                             size_t responselen)
 {
-    fd_set readfds;
+    GPollFD fds[1] = { {.fd = fd, .events = G_IO_IN } };
     int n;
-    struct timeval tv = {
-        .tv_sec = 1,
-        .tv_usec = 0,
-    };
 
     n = write(fd, request, requestlen);
     if (n < 0) {
@@ -127,11 +123,8 @@ static int tpm_util_request(int fd,
         return -EFAULT;
     }
 
-    FD_ZERO(&readfds);
-    FD_SET(fd, &readfds);
-
     /* wait for a second */
-    n = select(fd + 1, &readfds, NULL, NULL, &tv);
+    n = RETRY_ON_EINTR(g_poll(fds, 1, 1000));
     if (n != 1) {
         return -errno;
     }
-- 
2.41.0

Re: [PATCH v2] tpm: fix crash when FD >= 1024

Posted by Stefan Berger 7 months, 2 weeks ago

On 9/11/23 09:25, marcandre.lureau@redhat.com wrote:
> From: Marc-Andr޸ Lureau <marcandre.lureau@redhat.com>
>
> Replace select() with poll() to fix a crash when QEMU has a large number
> of FDs.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=2020133

For backporting I think we should also add this tag here:

Fixes:  ca64b08638 ("tpm: Move backend code under the 'backends/' 
directory")

Though RETRY_ON_EINTR was only introduced in 8.0.0-rc0. What's the right 
tag for backporting then?

    Stefan


> Signed-off-by: Marc-Andr޸ Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   backends/tpm/tpm_util.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
> index a6e6d3e72f..1856589c3b 100644
> --- a/backends/tpm/tpm_util.c
> +++ b/backends/tpm/tpm_util.c
> @@ -112,12 +112,8 @@ static int tpm_util_request(int fd,
>                               void *response,
>                               size_t responselen)
>   {
> -    fd_set readfds;
> +    GPollFD fds[1] = { {.fd = fd, .events = G_IO_IN } };
>       int n;
> -    struct timeval tv = {
> -        .tv_sec = 1,
> -        .tv_usec = 0,
> -    };
>
>       n = write(fd, request, requestlen);
>       if (n < 0) {
> @@ -127,11 +123,8 @@ static int tpm_util_request(int fd,
>           return -EFAULT;
>       }
>
> -    FD_ZERO(&readfds);
> -    FD_SET(fd, &readfds);
> -
>       /* wait for a second */
> -    n = select(fd + 1, &readfds, NULL, NULL, &tv);
> +    n = RETRY_ON_EINTR(g_poll(fds, 1, 1000));
>       if (n != 1) {
>           return -errno;
>       }

Re: [PATCH v2] tpm: fix crash when FD >= 1024

Posted by Michael Tokarev 7 months, 2 weeks ago

12.09.2023 03:08, Stefan Berger пишет:
> 
> On 9/11/23 09:25, marcandre.lureau@redhat.com wrote:
>> From: Marc-Andr޸ Lureau <marcandre.lureau@redhat.com>
>>
>> Replace select() with poll() to fix a crash when QEMU has a large number
>> of FDs.
>>
>> Fixes:
>> https://bugzilla.redhat.com/show_bug.cgi?id=2020133
> 
> Fixes:  ca64b08638 ("tpm: Move backend code under the 'backends/' directory")

Heh. I noticed this only now.  No, this is not the commit which introduced
the breakage.  It is either

commit 56a3c24ffc11955ddc7bb21362ca8069a3fc8c55
Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
Date:   Tue May 26 16:51:06 2015 -0400

     tpm: Probe for connected TPM 1.2 or TPM 2

which introduced select() in the first place (provided similar select()
hasn't been used in there before.  Or some other commit somewhere else
which allowed to have large number of filedescriptors - provided it wasn't
possible before.  But definitely not a commit which just moved file into
another subdir :)

/mjt

Re: [PATCH v2] tpm: fix crash when FD >= 1024

Posted by Michael Tokarev 7 months, 2 weeks ago

12.09.2023 03:08, Stefan Berger wrote:
> 
> On 9/11/23 09:25, marcandre.lureau@redhat.com wrote:
>> From: Marc-Andr޸ Lureau <marcandre.lureau@redhat.com>
>>
>> Replace select() with poll() to fix a crash when QEMU has a large number
>> of FDs.
>>
>> Fixes:
>> https://bugzilla.redhat.com/show_bug.cgi?id=2020133
> 
> For backporting I think we should also add this tag here:
> 
> Fixes:  ca64b08638 ("tpm: Move backend code under the 'backends/' directory")

It's nice to have Fixes tags generally.

Yes, it helps backporting a little bit, but it is mostly about choosing
which changes might be appropriate when there's no to-stable/to-backport
markers/tags whatsoever.  If you already know for sure some change should
be picked up for stable, it's better to add Cc: qemu-stable@.  With Fixes
also in place, besides its usefulness for other purposes, it helps me to
see which older versions needs this, but usually it's relatively easy to
determine even without Fixes: tag.  Many changes picked up for stable do
not have such tag just because there's no single commit which introduced
an issue, or some other situation.
> 
> Though RETRY_ON_EINTR was only introduced in 8.0.0-rc0. What's the right tag for backporting then?

There's no such tag.  If you know already there's possible issue with older
versions (and this is exactly the case), any comment about this might help
for sure.  This your note saved me a compile (which would fail for sure),
after which I would find

commit v7.2.0-538-g8b6aa69365
Author: Nikita Ivanov <nivanov@cloudlinux.com>
Date:   Sun Oct 23 12:04:21 2022 +0300

     Refactoring: refactor TFR() macro to RETRY_ON_EINTR()

the same way I did now.

If you're trying to find a way to make this new fix be "more backportable",
maybe by avoiding using a feature designed especially for this, - I think
this is not productive, the priority is definitely to have better "master",
and think about what to do with earlier versions only after that.

In this case, and in about 5 other examples from today, the thought about
stable releases best be done when introducing wide changes, like this commit
above which replaced TFR with RETRY_ON_EINTR.  Since this new macro will be
used everywhere for sure, the best way would be to split that single patch
into 3: first one introducing the new RETRY_ON_EINTR(), second converting all
users of TFR to RETRY_ON_EINTR, and 3rd (which can be folded into second)
removing TFR which is now unused.  This way I can cherry-pick just the first
patch easily if needed.  But once again, the priority should be master, not
backports.

Thanks,

/mjt