| Message ID | 20220624143912.1234427-1-mcascell@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/net/rocker: avoid NULL pointer dereference in of_dpa_cmd_add_l2_flood | expand |
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
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 > > >
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 >>> >> >
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 > >>> > >> > > >
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]); }
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(+)