[v1] hw/net/rocker: avoid NULL pointer dereference in of_dpa_cmd_add_l2_flood

[PATCH] hw/net/rocker: avoid NULL pointer dereference in of_dpa_cmd_add_l2_flood

Posted by Mauro Matteo Cascella 1 year, 11 months ago

rocker_tlv_parse_nested could return early because of no group ids in
the group_tlvs. In such case tlvs is NULL; tlvs[i + 1] in the next
for-loop will deref the NULL pointer.

Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
Reported-by: <arayz_w@icloud.com>
---
 hw/net/rocker/rocker_of_dpa.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
index b3b8c5bb6d..1611b79227 100644
--- a/hw/net/rocker/rocker_of_dpa.c
+++ b/hw/net/rocker/rocker_of_dpa.c
@@ -2039,6 +2039,11 @@ static int of_dpa_cmd_add_l2_flood(OfDpa *of_dpa, OfDpaGroup *group,
     rocker_tlv_parse_nested(tlvs, group->l2_flood.group_count,
                             group_tlvs[ROCKER_TLV_OF_DPA_GROUP_IDS]);
 
+    if (!tlvs) {
+        err = -ROCKER_EINVAL;
+        goto err_out;
+    }
+
     for (i = 0; i < group->l2_flood.group_count; i++) {
         group->l2_flood.group_ids[i] = rocker_tlv_get_le32(tlvs[i + 1]);
     }
-- 
2.35.3

Re: [PATCH] hw/net/rocker: avoid NULL pointer dereference in of_dpa_cmd_add_l2_flood

Posted by Mauro Matteo Cascella 8 months, 4 weeks ago

On Fri, Jun 24, 2022 at 4:40 PM Mauro Matteo Cascella
<mcascell@redhat.com> wrote:
>
> rocker_tlv_parse_nested could return early because of no group ids in
> the group_tlvs. In such case tlvs is NULL; tlvs[i + 1] in the next
> for-loop will deref the NULL pointer.

Someone somehow reserved a new CVE for this bug, published a few days
ago here: https://nvd.nist.gov/vuln/detail/CVE-2022-36648.

Not only is this not CVE worthy (rocker code does not fall under the
KVM virtualization use case [1]) but what's most concerning is that it
got a CVSS score of 10 :/

I'm going to dispute this CVE. Hopefully, it will be rejected soon. In
any case, can we get this patch merged?

[1] https://www.qemu.org/docs/master/system/security.html

Thanks,

> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> Reported-by: <arayz_w@icloud.com>
> ---
>  hw/net/rocker/rocker_of_dpa.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
> index b3b8c5bb6d..1611b79227 100644
> --- a/hw/net/rocker/rocker_of_dpa.c
> +++ b/hw/net/rocker/rocker_of_dpa.c
> @@ -2039,6 +2039,11 @@ static int of_dpa_cmd_add_l2_flood(OfDpa *of_dpa, OfDpaGroup *group,
>      rocker_tlv_parse_nested(tlvs, group->l2_flood.group_count,
>                              group_tlvs[ROCKER_TLV_OF_DPA_GROUP_IDS]);
>
> +    if (!tlvs) {
> +        err = -ROCKER_EINVAL;
> +        goto err_out;
> +    }
> +
>      for (i = 0; i < group->l2_flood.group_count; i++) {
>          group->l2_flood.group_ids[i] = rocker_tlv_get_le32(tlvs[i + 1]);
>      }
> --
> 2.35.3
>

--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH] hw/net/rocker: avoid NULL pointer dereference in of_dpa_cmd_add_l2_flood

Posted by Mauro Matteo Cascella 8 months, 4 weeks ago

On Sat, Aug 26, 2023 at 4:31 PM Mauro Matteo Cascella
<mcascell@redhat.com> wrote:
>
> On Fri, Jun 24, 2022 at 4:40 PM Mauro Matteo Cascella
> <mcascell@redhat.com> wrote:
> >
> > rocker_tlv_parse_nested could return early because of no group ids in
> > the group_tlvs. In such case tlvs is NULL; tlvs[i + 1] in the next
> > for-loop will deref the NULL pointer.

Looking at the code once again, tlvs is a pointer to a g_new0
allocated memory, so I don't know how it can be NULL after
rocker_tlv_parse_nested (unless g_new0 returns NULL in the first
place). I do not recall the details of this bug. Arayz?

> Someone somehow reserved a new CVE for this bug, published a few days
> ago here: https://nvd.nist.gov/vuln/detail/CVE-2022-36648.
>
> Not only is this not CVE worthy (rocker code does not fall under the
> KVM virtualization use case [1]) but what's most concerning is that it
> got a CVSS score of 10 :/
>
> I'm going to dispute this CVE. Hopefully, it will be rejected soon. In
> any case, can we get this patch merged?
>
> [1] https://www.qemu.org/docs/master/system/security.html
>
> Thanks,
>
> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > Reported-by: <arayz_w@icloud.com>
> > ---
> >  hw/net/rocker/rocker_of_dpa.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
> > index b3b8c5bb6d..1611b79227 100644
> > --- a/hw/net/rocker/rocker_of_dpa.c
> > +++ b/hw/net/rocker/rocker_of_dpa.c
> > @@ -2039,6 +2039,11 @@ static int of_dpa_cmd_add_l2_flood(OfDpa *of_dpa, OfDpaGroup *group,
> >      rocker_tlv_parse_nested(tlvs, group->l2_flood.group_count,
> >                              group_tlvs[ROCKER_TLV_OF_DPA_GROUP_IDS]);
> >
> > +    if (!tlvs) {
> > +        err = -ROCKER_EINVAL;
> > +        goto err_out;
> > +    }
> > +
> >      for (i = 0; i < group->l2_flood.group_count; i++) {
> >          group->l2_flood.group_ids[i] = rocker_tlv_get_le32(tlvs[i + 1]);
> >      }
> > --
> > 2.35.3
> >
>

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH] hw/net/rocker: avoid NULL pointer dereference in of_dpa_cmd_add_l2_flood

Posted by Philippe Mathieu-Daudé 8 months, 3 weeks ago

On 27/8/23 13:07, Mauro Matteo Cascella wrote:
> On Sat, Aug 26, 2023 at 4:31 PM Mauro Matteo Cascella
> <mcascell@redhat.com> wrote:
>>
>> On Fri, Jun 24, 2022 at 4:40 PM Mauro Matteo Cascella
>> <mcascell@redhat.com> wrote:
>>>
>>> rocker_tlv_parse_nested could return early because of no group ids in
>>> the group_tlvs. In such case tlvs is NULL; tlvs[i + 1] in the next
>>> for-loop will deref the NULL pointer.
> 
> Looking at the code once again, tlvs is a pointer to a g_new0
> allocated memory, so I don't know how it can be NULL after
> rocker_tlv_parse_nested (unless g_new0 returns NULL in the first
> place). I do not recall the details of this bug. Arayz?

Per <glib.h>:

   If any call to allocate memory using functions g_new(), g_new0(),
   g_renew(), g_malloc(), g_malloc0(), g_malloc0_n(), g_realloc(), and
   g_realloc_n() fails, the application is terminated. This also means
   that there is no need to check if the call succeeded. On the other
   hand, g_try_...() family of functions returns NULL on failure that
   can be used as a check for unsuccessful memory allocation. The
   application is not terminated in this case.


group->l2_flood.group_count is a uint16_t, so up to UINT16_MAX = 0xffff.

So:

   tlvs = g_new0(RockerTlv *, group->l2_flood.group_count + 1);

is at most an malloc(0x10000 * sizeof(void *)) = 0x80000 = 512 KiB.

QEMU use way bigger heap allocations.

I don't know the net/ subsystem enough to have an idea about how many
concurrent packets can be processed by this device model, but I'd be
surprised if this triggers a OOM.

As usual, do you have a reproducer?

>> Someone somehow reserved a new CVE for this bug, published a few days
>> ago here: https://nvd.nist.gov/vuln/detail/CVE-2022-36648.
>>
>> Not only is this not CVE worthy (rocker code does not fall under the
>> KVM virtualization use case [1]) but what's most concerning is that it
>> got a CVSS score of 10 :/
>>
>> I'm going to dispute this CVE. Hopefully, it will be rejected soon. In
>> any case, can we get this patch merged?
>>
>> [1] https://www.qemu.org/docs/master/system/security.html
>>
>> Thanks,
>>
>>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>>> Reported-by: <arayz_w@icloud.com>
>>> ---
>>>   hw/net/rocker/rocker_of_dpa.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
>>> index b3b8c5bb6d..1611b79227 100644
>>> --- a/hw/net/rocker/rocker_of_dpa.c
>>> +++ b/hw/net/rocker/rocker_of_dpa.c
>>> @@ -2039,6 +2039,11 @@ static int of_dpa_cmd_add_l2_flood(OfDpa *of_dpa, OfDpaGroup *group,
>>>       rocker_tlv_parse_nested(tlvs, group->l2_flood.group_count,
>>>                               group_tlvs[ROCKER_TLV_OF_DPA_GROUP_IDS]);
>>>
>>> +    if (!tlvs) {
>>> +        err = -ROCKER_EINVAL;
>>> +        goto err_out;
>>> +    }
>>> +
>>>       for (i = 0; i < group->l2_flood.group_count; i++) {
>>>           group->l2_flood.group_ids[i] = rocker_tlv_get_le32(tlvs[i + 1]);
>>>       }
>>> --
>>> 2.35.3
>>>
>>
>

Re: [PATCH] hw/net/rocker: avoid NULL pointer dereference in of_dpa_cmd_add_l2_flood

Posted by Mauro Matteo Cascella 8 months, 3 weeks ago

On Mon, Aug 28, 2023 at 6:11 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 27/8/23 13:07, Mauro Matteo Cascella wrote:
> > On Sat, Aug 26, 2023 at 4:31 PM Mauro Matteo Cascella
> > <mcascell@redhat.com> wrote:
> >>
> >> On Fri, Jun 24, 2022 at 4:40 PM Mauro Matteo Cascella
> >> <mcascell@redhat.com> wrote:
> >>>
> >>> rocker_tlv_parse_nested could return early because of no group ids in
> >>> the group_tlvs. In such case tlvs is NULL; tlvs[i + 1] in the next
> >>> for-loop will deref the NULL pointer.
> >
> > Looking at the code once again, tlvs is a pointer to a g_new0
> > allocated memory, so I don't know how it can be NULL after
> > rocker_tlv_parse_nested (unless g_new0 returns NULL in the first
> > place). I do not recall the details of this bug. Arayz?
>
> Per <glib.h>:
>
>    If any call to allocate memory using functions g_new(), g_new0(),
>    g_renew(), g_malloc(), g_malloc0(), g_malloc0_n(), g_realloc(), and
>    g_realloc_n() fails, the application is terminated. This also means
>    that there is no need to check if the call succeeded. On the other
>    hand, g_try_...() family of functions returns NULL on failure that
>    can be used as a check for unsuccessful memory allocation. The
>    application is not terminated in this case.
>
>
> group->l2_flood.group_count is a uint16_t, so up to UINT16_MAX = 0xffff.
>
> So:
>
>    tlvs = g_new0(RockerTlv *, group->l2_flood.group_count + 1);
>
> is at most an malloc(0x10000 * sizeof(void *)) = 0x80000 = 512 KiB.
>
> QEMU use way bigger heap allocations.
>
> I don't know the net/ subsystem enough to have an idea about how many
> concurrent packets can be processed by this device model, but I'd be
> surprised if this triggers a OOM.
>
> As usual, do you have a reproducer?

I just found the original bug report and opened a new issue upstream.
See https://gitlab.com/qemu-project/qemu/-/issues/1851.

> >> Someone somehow reserved a new CVE for this bug, published a few days
> >> ago here: https://nvd.nist.gov/vuln/detail/CVE-2022-36648.
> >>
> >> Not only is this not CVE worthy (rocker code does not fall under the
> >> KVM virtualization use case [1]) but what's most concerning is that it
> >> got a CVSS score of 10 :/
> >>
> >> I'm going to dispute this CVE. Hopefully, it will be rejected soon. In
> >> any case, can we get this patch merged?
> >>
> >> [1] https://www.qemu.org/docs/master/system/security.html
> >>
> >> Thanks,
> >>
> >>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> >>> Reported-by: <arayz_w@icloud.com>
> >>> ---
> >>>   hw/net/rocker/rocker_of_dpa.c | 5 +++++
> >>>   1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
> >>> index b3b8c5bb6d..1611b79227 100644
> >>> --- a/hw/net/rocker/rocker_of_dpa.c
> >>> +++ b/hw/net/rocker/rocker_of_dpa.c
> >>> @@ -2039,6 +2039,11 @@ static int of_dpa_cmd_add_l2_flood(OfDpa *of_dpa, OfDpaGroup *group,
> >>>       rocker_tlv_parse_nested(tlvs, group->l2_flood.group_count,
> >>>                               group_tlvs[ROCKER_TLV_OF_DPA_GROUP_IDS]);
> >>>
> >>> +    if (!tlvs) {
> >>> +        err = -ROCKER_EINVAL;
> >>> +        goto err_out;
> >>> +    }
> >>> +
> >>>       for (i = 0; i < group->l2_flood.group_count; i++) {
> >>>           group->l2_flood.group_ids[i] = rocker_tlv_get_le32(tlvs[i + 1]);
> >>>       }
> >>> --
> >>> 2.35.3
> >>>
> >>
> >
>

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0