Fix UB in choose_service_fd_base due to calling __builtin_clz(0)

Submitted by Radoslaw Burny on April 9, 2018, 2:57 p.m.

Details

Message ID CAFkxGoPo_-YNGmq3trtGOnxBArTT5FuF9cb9T1jBZmVXoA2d_A@mail.gmail.com
State New
Series "Fix UB in choose_service_fd_base due to calling __builtin_clz(0)"
Headers show

Commit Message

Radoslaw Burny April 9, 2018, 2:57 p.m.
From: Radoslaw Burny <rburny@google.com>

Subject: [PATCH] Fix UB in choose_service_fd_base.

Signed-off-by: Radoslaw Burny <rburny@google.com>

---
 criu/util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

                /* Right border is bigger, than rlim. OK, then just aligned
value is enough */

Patch hide | download patch | download mbox

diff --git a/criu/util.c b/criu/util.c
index b19bf517..48ba09a8 100644
--- a/criu/util.c
+++ b/criu/util.c
@@ -588,9 +588,9 @@  static int choose_service_fd_base(struct pstree_item
*me)
        nr += 16; /* Safety pad */
        real_nr = nr;

-       nr /= (1024 / sizeof(void *));
+       /* Align nr to the power of 2 for easier debugging */
+       BUG_ON(nr <= 0);
        nr = 1 << (32 - __builtin_clz(nr));
-       nr *= (1024 / sizeof(void *));

        if (nr > service_fd_rlim_cur) {

Comments

Kirill Tkhai April 9, 2018, 4:03 p.m.
On 09.04.2018 17:57, Radoslaw Burny wrote:
> From: Radoslaw Burny <rburny@google.com>
> 
> Subject: [PATCH] Fix UB in choose_service_fd_base.
> 
> Signed-off-by: Radoslaw Burny <rburny@google.com>
> 
> ---
>  criu/util.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/criu/util.c b/criu/util.c
> index b19bf517..48ba09a8 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -588,9 +588,9 @@ static int choose_service_fd_base(struct pstree_item
> *me)
>         nr += 16; /* Safety pad */
>         real_nr = nr;
> 
> -       nr /= (1024 / sizeof(void *));
> +       /* Align nr to the power of 2 for easier debugging */
> +       BUG_ON(nr <= 0);
>         nr = 1 << (32 - __builtin_clz(nr));
> -       nr *= (1024 / sizeof(void *));
> 
>         if (nr > service_fd_rlim_cur) {
>                 /* Right border is bigger, than rlim. OK, then just aligned
> value is enough */

It's not a round up power of 2, it's round up power of 2 rounded to 128.
This goes from kernel: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/file.c#n85

#include <limits.h>
int main(void)
{
unsigned int i, was, nr;

    for (i = 0; i < INT_MAX; i++) {
            nr = i;
            nr /= (1024 / sizeof(void *));
            if (nr)
                    nr = 1 << (32 - __builtin_clz(nr));
            else
                    nr = 1;
            nr *= (1024 / sizeof(void *));

            printf("nr=%d, i=%d\n", nr, i);
            if (nr < i || (nr-1) & nr) {
                    printf("error\n");
                    exit(1);
            }
    }

    return 0;

}

nr=128, i=16
nr=128, i=17
nr=128, i=18
nr=128, i=19
nr=128, i=20
nr=128, i=21
nr=128, i=22
nr=128, i=23
nr=128, i=24
nr=128, i=25
nr=128, i=26
nr=128, i=27
nr=128, i=28
nr=128, i=29
nr=128, i=30
Radoslaw Burny April 9, 2018, 4:16 p.m.
Ah, I see. Even with the fds that CRIU adds, it's still possible to fit
below 64 or even 32.
I've reverted the patch to the version you suggested and attached it below
- I presume this is the code review process for CRIU, right?

BTW, if you want to make any changes to the patch before merging it -
feel free to do so :)
It will be faster than us two iterating over the email.

Thanks!

On Mon, Apr 9, 2018 at 6:03 PM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:

> On 09.04.2018 17:57, Radoslaw Burny wrote:
> > From: Radoslaw Burny <rburny@google.com>
> >
> > Subject: [PATCH] Fix UB in choose_service_fd_base.
> >
> > Signed-off-by: Radoslaw Burny <rburny@google.com>
> >
> > ---
> >  criu/util.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/criu/util.c b/criu/util.c
> > index b19bf517..48ba09a8 100644
> > --- a/criu/util.c
> > +++ b/criu/util.c
> > @@ -588,9 +588,9 @@ static int choose_service_fd_base(struct pstree_item
> > *me)
> >         nr += 16; /* Safety pad */
> >         real_nr = nr;
> >
> > -       nr /= (1024 / sizeof(void *));
> > +       /* Align nr to the power of 2 for easier debugging */
> > +       BUG_ON(nr <= 0);
> >         nr = 1 << (32 - __builtin_clz(nr));
> > -       nr *= (1024 / sizeof(void *));
> >
> >         if (nr > service_fd_rlim_cur) {
> >                 /* Right border is bigger, than rlim. OK, then just
> aligned
> > value is enough */
>
> It's not a round up power of 2, it's round up power of 2 rounded to 128.
> This goes from kernel: https://git.kernel.org/pub/
> scm/linux/kernel/git/torvalds/linux.git/tree/fs/file.c#n85
>
> #include <limits.h>
> int main(void)
> {
> unsigned int i, was, nr;
>
>     for (i = 0; i < INT_MAX; i++) {
>             nr = i;
>             nr /= (1024 / sizeof(void *));
>             if (nr)
>                     nr = 1 << (32 - __builtin_clz(nr));
>             else
>                     nr = 1;
>             nr *= (1024 / sizeof(void *));
>
>             printf("nr=%d, i=%d\n", nr, i);
>             if (nr < i || (nr-1) & nr) {
>                     printf("error\n");
>                     exit(1);
>             }
>     }
>
>     return 0;
>
> }
>
> nr=128, i=16
> nr=128, i=17
> nr=128, i=18
> nr=128, i=19
> nr=128, i=20
> nr=128, i=21
> nr=128, i=22
> nr=128, i=23
> nr=128, i=24
> nr=128, i=25
> nr=128, i=26
> nr=128, i=27
> nr=128, i=28
> nr=128, i=29
> nr=128, i=30
>



From: Radoslaw Burny <rburny at google.com>



Subject: [PATCH] Fix UB in choose_service_fd_base (rev 2).





Signed-off-by: Radoslaw Burny <rburny at google.com>



---
 criu/util.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/criu/util.c b/criu/util.c
index b19bf517..56a1683e 100644
--- a/criu/util.c

+++ b/criu/util.c

@@ -589,7 +589,10 @@ static int choose_service_fd_base(struct pstree_item
*me)
        real_nr = nr;

        nr /= (1024 / sizeof(void *));
-       nr = 1 << (32 - __builtin_clz(nr));
+       if (nr != 0)
+               nr = 1 << (32 - __builtin_clz(nr));
+       else
+               nr = 1;
        nr *= (1024 / sizeof(void *));

        if (nr > service_fd_rlim_cur) {
--
2.17.0.484.g0c8726318c-goog
Kirill Tkhai April 9, 2018, 4:19 p.m.
On 09.04.2018 19:16, Radoslaw Burny wrote:
> Ah, I see. Even with the fds that CRIU adds, it's still possible to fit
> below 64 or even 32.
> I've reverted the patch to the version you suggested and attached it below
> - I presume this is the code review process for CRIU, right?

I assume, criu patchwork will be able to pick this patch up from the reply. Andrew, is this OK?

> BTW, if you want to make any changes to the patch before merging it -
> feel free to do so :)
> It will be faster than us two iterating over the email.
> 
> Thanks!

Thanks,
Kirill
 
> On Mon, Apr 9, 2018 at 6:03 PM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> 
>> On 09.04.2018 17:57, Radoslaw Burny wrote:
>>> From: Radoslaw Burny <rburny@google.com>
>>>
>>> Subject: [PATCH] Fix UB in choose_service_fd_base.
>>>
>>> Signed-off-by: Radoslaw Burny <rburny@google.com>
>>>
>>> ---
>>>  criu/util.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/criu/util.c b/criu/util.c
>>> index b19bf517..48ba09a8 100644
>>> --- a/criu/util.c
>>> +++ b/criu/util.c
>>> @@ -588,9 +588,9 @@ static int choose_service_fd_base(struct pstree_item
>>> *me)
>>>         nr += 16; /* Safety pad */
>>>         real_nr = nr;
>>>
>>> -       nr /= (1024 / sizeof(void *));
>>> +       /* Align nr to the power of 2 for easier debugging */
>>> +       BUG_ON(nr <= 0);
>>>         nr = 1 << (32 - __builtin_clz(nr));
>>> -       nr *= (1024 / sizeof(void *));
>>>
>>>         if (nr > service_fd_rlim_cur) {
>>>                 /* Right border is bigger, than rlim. OK, then just
>> aligned
>>> value is enough */
>>
>> It's not a round up power of 2, it's round up power of 2 rounded to 128.
>> This goes from kernel: https://git.kernel.org/pub/
>> scm/linux/kernel/git/torvalds/linux.git/tree/fs/file.c#n85
>>
>> #include <limits.h>
>> int main(void)
>> {
>> unsigned int i, was, nr;
>>
>>     for (i = 0; i < INT_MAX; i++) {
>>             nr = i;
>>             nr /= (1024 / sizeof(void *));
>>             if (nr)
>>                     nr = 1 << (32 - __builtin_clz(nr));
>>             else
>>                     nr = 1;
>>             nr *= (1024 / sizeof(void *));
>>
>>             printf("nr=%d, i=%d\n", nr, i);
>>             if (nr < i || (nr-1) & nr) {
>>                     printf("error\n");
>>                     exit(1);
>>             }
>>     }
>>
>>     return 0;
>>
>> }
>>
>> nr=128, i=16
>> nr=128, i=17
>> nr=128, i=18
>> nr=128, i=19
>> nr=128, i=20
>> nr=128, i=21
>> nr=128, i=22
>> nr=128, i=23
>> nr=128, i=24
>> nr=128, i=25
>> nr=128, i=26
>> nr=128, i=27
>> nr=128, i=28
>> nr=128, i=29
>> nr=128, i=30
>>
> 
> 
> 
> From: Radoslaw Burny <rburny at google.com>
> 
> 
> 
> Subject: [PATCH] Fix UB in choose_service_fd_base (rev 2).
> 
> 
> 
> 
> 
> Signed-off-by: Radoslaw Burny <rburny at google.com>
> 
> 
> 
> ---
>  criu/util.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/util.c b/criu/util.c
> index b19bf517..56a1683e 100644
> --- a/criu/util.c
> 
> +++ b/criu/util.c
> 
> @@ -589,7 +589,10 @@ static int choose_service_fd_base(struct pstree_item
> *me)
>         real_nr = nr;
> 
>         nr /= (1024 / sizeof(void *));
> -       nr = 1 << (32 - __builtin_clz(nr));
> +       if (nr != 0)
> +               nr = 1 << (32 - __builtin_clz(nr));
> +       else
> +               nr = 1;
>         nr *= (1024 / sizeof(void *));
> 
>         if (nr > service_fd_rlim_cur) {
> --
> 2.17.0.484.g0c8726318c-goog
>