[Qemu-devel] [Qemu-devel PATCH v2 0/2] Fix incorrect assertions in sd and main-loop

[Qemu-devel] [Qemu-devel PATCH v2 0/2] Fix incorrect assertions in sd and main-loop

[Lidong Chen]

v1: Patch1 fixed out-of-bounds assertions in hw/sd/sd.c

v2: Added patch2 that fixed the incorrect assertion for poll_fds
    in util/main-loop.c based on the feedbacks from Peter Maydell
    and Liam Merwick,

Lidong Chen (2):
  sd: Fix out-of-bounds assertions
  util/main-loop: Fix incorrect assertion

 hw/sd/sd.c       | 4 ++--
 util/main-loop.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [Qemu-devel PATCH v2 1/2] sd: Fix out-of-bounds assertions

[Lidong Chen]

Due to an off-by-one error, the assert statements allow an
out-of-bound array access.

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
---
 hw/sd/sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index aaab15f..818f86c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
     if (state == sd_inactive_state) {
         return "inactive";
     }
-    assert(state <= ARRAY_SIZE(state_name));
+    assert(state < ARRAY_SIZE(state_name));
     return state_name[state];
 }
 
@@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
     if (rsp == sd_r1b) {
         rsp = sd_r1;
     }
-    assert(rsp <= ARRAY_SIZE(response_name));
+    assert(rsp < ARRAY_SIZE(response_name));
     return response_name[rsp];
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-devel PATCH v2 1/2] sd: Fix out-of-bounds assertions

[Philippe Mathieu-Daudé]

Hi,

On 6/5/19 9:15 PM, Lidong Chen wrote:
> Due to an off-by-one error, the assert statements allow an
> out-of-bound array access.

I believe this is a "v2 RESEND" patch, since there is no change with the
other v2 posted here:
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00634.html
So I'll copy paste the same comment I wrote there:

  Not sure via which tree this patch is going (trivial?).
  To the maintainer, please consider adding when applying:

  "This access can not happen. Fix to silent static analyzer warnings."

  As confirmed by Lidong in v1 here:
  https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg01337.html

Thanks,

Phil.

> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
>  hw/sd/sd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index aaab15f..818f86c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -144,7 +144,7 @@ static const char *sd_state_name(enum SDCardStates state)
>      if (state == sd_inactive_state) {
>          return "inactive";
>      }
> -    assert(state <= ARRAY_SIZE(state_name));
> +    assert(state < ARRAY_SIZE(state_name));
>      return state_name[state];
>  }
>  
> @@ -165,7 +165,7 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
>      if (rsp == sd_r1b) {
>          rsp = sd_r1;
>      }
> -    assert(rsp <= ARRAY_SIZE(response_name));
> +    assert(rsp < ARRAY_SIZE(response_name));
>      return response_name[rsp];
>  }
>  
> 

[Qemu-devel] [Qemu-devel PATCH v2 2/2] util/main-loop: Fix incorrect assertion

[Lidong Chen]

The check for poll_fds in g_assert() was incorrect. The correct assertion
should check "n_poll_fds + w->num <= ARRAY_SIZE(poll_fds)" because the
subsequent for-loop is doing access to poll_fds[n_poll_fds + i] where i
is in [0, w->num).

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Liam Merwick <liam.merwick@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
---
 util/main-loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/main-loop.c b/util/main-loop.c
index e1e349c..a9f4e8d 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -422,7 +422,7 @@ static int os_host_main_loop_wait(int64_t timeout)
     g_main_context_prepare(context, &max_priority);
     n_poll_fds = g_main_context_query(context, max_priority, &poll_timeout,
                                       poll_fds, ARRAY_SIZE(poll_fds));
-    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
+    g_assert(n_poll_fds + w->num <= ARRAY_SIZE(poll_fds));
 
     for (i = 0; i < w->num; i++) {
         poll_fds[n_poll_fds + i].fd = (DWORD_PTR)w->events[i];
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-devel PATCH v2 2/2] util/main-loop: Fix incorrect assertion

[Markus Armbruster]

You neglected to cc: the file's maintainer.  I'm doing that for you now.
In the future, use scripts/get_maintainer.pl to find maintainers you
might want to cc:.

Lidong Chen <lidong.chen@oracle.com> writes:

> The check for poll_fds in g_assert() was incorrect. The correct assertion
> should check "n_poll_fds + w->num <= ARRAY_SIZE(poll_fds)" because the
> subsequent for-loop is doing access to poll_fds[n_poll_fds + i] where i
> is in [0, w->num).
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Liam Merwick <liam.merwick@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> ---
>  util/main-loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/main-loop.c b/util/main-loop.c
> index e1e349c..a9f4e8d 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -422,7 +422,7 @@ static int os_host_main_loop_wait(int64_t timeout)
>      g_main_context_prepare(context, &max_priority);
>      n_poll_fds = g_main_context_query(context, max_priority, &poll_timeout,
>                                        poll_fds, ARRAY_SIZE(poll_fds));
> -    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> +    g_assert(n_poll_fds + w->num <= ARRAY_SIZE(poll_fds));
>  
>      for (i = 0; i < w->num; i++) {
>          poll_fds[n_poll_fds + i].fd = (DWORD_PTR)w->events[i];

Re: [Qemu-devel] [Qemu-devel PATCH v2 2/2] util/main-loop: Fix incorrect assertion

[Philippe Mathieu-Daudé]

On 6/5/19 9:15 PM, Lidong Chen wrote:
> The check for poll_fds in g_assert() was incorrect. The correct assertion
> should check "n_poll_fds + w->num <= ARRAY_SIZE(poll_fds)" because the
> subsequent for-loop is doing access to poll_fds[n_poll_fds + i] where i
> is in [0, w->num).
> 
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Liam Merwick <liam.merwick@oracle.com>

Ah, so this is not a plain "v2 RESEND" patch of
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00636.html
since you added Peter 'Suggested-by' tag...

> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> Reviewed-by: Li Qiang <liq3ea@gmail.com>

... but then you dropped my 'Reviewed-by'. Assuming this is a typo, and
since there is no logical change in this patch with the previous one,
here it goes again:

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Regards,

Phil.

> ---
>  util/main-loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/main-loop.c b/util/main-loop.c
> index e1e349c..a9f4e8d 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -422,7 +422,7 @@ static int os_host_main_loop_wait(int64_t timeout)
>      g_main_context_prepare(context, &max_priority);
>      n_poll_fds = g_main_context_query(context, max_priority, &poll_timeout,
>                                        poll_fds, ARRAY_SIZE(poll_fds));
> -    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> +    g_assert(n_poll_fds + w->num <= ARRAY_SIZE(poll_fds));
>  
>      for (i = 0; i < w->num; i++) {
>          poll_fds[n_poll_fds + i].fd = (DWORD_PTR)w->events[i];
> 

Re: [Qemu-devel] [Qemu-devel PATCH v2 2/2] util/main-loop: Fix incorrect assertion

[Lidong Chen]

Hi Philippe,

On 6/6/2019 4:18 PM, Philippe Mathieu-Daudé wrote:
> On 6/5/19 9:15 PM, Lidong Chen wrote:
>> The check for poll_fds in g_assert() was incorrect. The correct assertion
>> should check "n_poll_fds + w->num <= ARRAY_SIZE(poll_fds)" because the
>> subsequent for-loop is doing access to poll_fds[n_poll_fds + i] where i
>> is in [0, w->num).
>>
>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Suggested-by: Liam Merwick <liam.merwick@oracle.com>
> Ah, so this is not a plain "v2 RESEND" patch of
> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg00636.html
> since you added Peter 'Suggested-by' tag...
So,  should I resend it as v3 patch?
>> Reviewed-by: Liran Alon <liran.alon@oracle.com>
>> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
>> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> ... but then you dropped my 'Reviewed-by'. Assuming this is a typo, and
> since there is no logical change in this patch with the previous one,
> here it goes again:
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks for catching that!  Sorry about it, I will add your 'Reviewed-by'.

Thanks,

Lidong

>
> Regards,
>
> Phil.
>
>> ---
>>   util/main-loop.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/util/main-loop.c b/util/main-loop.c
>> index e1e349c..a9f4e8d 100644
>> --- a/util/main-loop.c
>> +++ b/util/main-loop.c
>> @@ -422,7 +422,7 @@ static int os_host_main_loop_wait(int64_t timeout)
>>       g_main_context_prepare(context, &max_priority);
>>       n_poll_fds = g_main_context_query(context, max_priority, &poll_timeout,
>>                                         poll_fds, ARRAY_SIZE(poll_fds));
>> -    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
>> +    g_assert(n_poll_fds + w->num <= ARRAY_SIZE(poll_fds));
>>   
>>       for (i = 0; i < w->num; i++) {
>>           poll_fds[n_poll_fds + i].fd = (DWORD_PTR)w->events[i];
>>