Re: [PATCH v3 0/2] hw/usb: add configuration flags for emulated and passthru usb smartcard
On 2022-12-13 16:02, Stefan Hajnoczi wrote: On Mon, Dec 12, 2022 at 05:09:47PM -0500, Jon Maloy wrote: We add three new configuration flags, LIBCACARD, USB_SMARTCARD_PASSTHRU and USB_SMARTCARD_EMULATED in order to improve configurability of these functionalities. Signed-off-by: Jon Maloy --- v2: Added a LIBACARD flag, plus reversed 'select' clauses, as suggested by Paolo Bonzini and Marc-André Lureau. v3: Split in two commits, so that LIBCACARD is added separately, as suggested by Philippe Mathieu-Daudé. Jon Maloy (2): hw/usb: add configuration flags for emulated and passthru usb smartcard hw/usb: add configuration flag for Common Access Card library code Kconfig.host | 3 +++ hw/usb/Kconfig | 14 ++ hw/usb/meson.build | 11 --- meson.build| 1 + 4 files changed, 22 insertions(+), 7 deletions(-) -- 2.35.3 I have CCed Gerd Hoffmann since he is the hw/usb/ maintainer. Reviewed-by: Stefan Hajnoczi Hi Gerd, It seems this one was forgotten, and never applied. Could you have a look? Thanks ///jon
[PATCH v3 1/2] hw/usb: add configuration flags for emulated and passthru usb smartcard
We add two new configuration flags, USB_SMARTCARD_PASSTHRU and
USB_SMARTCARD_EMULATED in order to improve configurability of these
functionalities.
Signed-off-by: Jon Maloy
---
hw/usb/Kconfig | 12
hw/usb/meson.build | 4 ++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index ce4f433976..6b29e91593 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -109,6 +109,18 @@ config USB_SMARTCARD
default y
depends on USB
+config USB_SMARTCARD_PASSTHRU
+bool
+default y
+depends on USB
+select USB_SMARTCARD
+
+config USB_SMARTCARD_EMULATED
+bool
+default y
+depends on USB
+select USB_SMARTCARD
+
config USB_STORAGE_MTP
bool
default y
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index 793df42e21..353006fb6c 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -51,8 +51,8 @@ softmmu_ss.add(when: 'CONFIG_USB_SMARTCARD', if_true:
files('dev-smartcard-reade
if cacard.found()
usbsmartcard_ss = ss.source_set()
- usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD',
- if_true: [cacard, files('ccid-card-emulated.c',
'ccid-card-passthru.c')])
+ usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD_EMULATED', if_true: [cacard,
files('ccid-card-emulated.c')])
+ usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD_PASSTHRU', if_true: [cacard,
files('ccid-card-passthru.c')])
hw_usb_modules += {'smartcard': usbsmartcard_ss}
endif
--
2.35.3
[PATCH v3 2/2] hw/usb: add configuration flag for Common Access Card library code
We add a new configuration flag, LIBCACARD, indicating availability of the
libcacard code for building. This way, we can eliminate the explicit test
for cacard.found() when configuring
USB_SMARTCARD_EMULATED/USB_SMARTCARD_PASSTHRU
in hw/usb/meson.build.
Signed-off-by: Jon Maloy
---
Kconfig.host | 3 +++
hw/usb/Kconfig | 2 ++
hw/usb/meson.build | 11 ---
meson.build| 1 +
4 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/Kconfig.host b/Kconfig.host
index d763d89269..d7fd4a2203 100644
--- a/Kconfig.host
+++ b/Kconfig.host
@@ -39,6 +39,9 @@ config MULTIPROCESS_ALLOWED
bool
imply MULTIPROCESS
+config LIBCACARD
+bool
+
config FUZZ
bool
select SPARSE_MEM
diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index 6b29e91593..5c3da7c34d 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -113,12 +113,14 @@ config USB_SMARTCARD_PASSTHRU
bool
default y
depends on USB
+depends on LIBCACARD
select USB_SMARTCARD
config USB_SMARTCARD_EMULATED
bool
default y
depends on USB
+depends on LIBCACARD
select USB_SMARTCARD
config USB_STORAGE_MTP
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index 353006fb6c..499be1122c 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -48,13 +48,10 @@ softmmu_ss.add(when: ['CONFIG_POSIX',
'CONFIG_USB_STORAGE_MTP'], if_true: files(
# smartcard
softmmu_ss.add(when: 'CONFIG_USB_SMARTCARD', if_true:
files('dev-smartcard-reader.c'))
-
-if cacard.found()
- usbsmartcard_ss = ss.source_set()
- usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD_EMULATED', if_true: [cacard,
files('ccid-card-emulated.c')])
- usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD_PASSTHRU', if_true: [cacard,
files('ccid-card-passthru.c')])
- hw_usb_modules += {'smartcard': usbsmartcard_ss}
-endif
+usbsmartcard_ss = ss.source_set()
+usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD_EMULATED', if_true: [cacard,
files('ccid-card-emulated.c')])
+usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD_PASSTHRU', if_true: [cacard,
files('ccid-card-passthru.c')])
+hw_usb_modules += {'smartcard': usbsmartcard_ss}
# U2F
softmmu_ss.add(when: 'CONFIG_USB_U2F', if_true: files('u2f.c'))
diff --git a/meson.build b/meson.build
index 5c6b5a1c75..10e9b77ec1 100644
--- a/meson.build
+++ b/meson.build
@@ -2493,6 +2493,7 @@ have_ivshmem = config_host_data.get('CONFIG_EVENTFD')
host_kconfig = \
(get_option('fuzzing') ? ['CONFIG_FUZZ=y'] : []) + \
(have_tpm ? ['CONFIG_TPM=y'] : []) + \
+ (cacard.found() ? ['CONFIG_LIBCACARD=y'] : []) + \
(spice.found() ? ['CONFIG_SPICE=y'] : []) + \
(have_ivshmem ? ['CONFIG_IVSHMEM=y'] : []) + \
(opengl.found() ? ['CONFIG_OPENGL=y'] : []) + \
--
2.35.3
[PATCH v3 0/2] hw/usb: add configuration flags for emulated and passthru usb smartcard
We add three new configuration flags, LIBCACARD, USB_SMARTCARD_PASSTHRU and USB_SMARTCARD_EMULATED in order to improve configurability of these functionalities. Signed-off-by: Jon Maloy --- v2: Added a LIBACARD flag, plus reversed 'select' clauses, as suggested by Paolo Bonzini and Marc-André Lureau. v3: Split in two commits, so that LIBCACARD is added separately, as suggested by Philippe Mathieu-Daudé. Jon Maloy (2): hw/usb: add configuration flags for emulated and passthru usb smartcard hw/usb: add configuration flag for Common Access Card library code Kconfig.host | 3 +++ hw/usb/Kconfig | 14 ++ hw/usb/meson.build | 11 --- meson.build| 1 + 4 files changed, 22 insertions(+), 7 deletions(-) -- 2.35.3
Re: [PATCH] SecurityPkg: check return value of GetEfiGlobalVariable2() in DxeImageVerificationHandler()
Please ignore this one. Sent to the wrong list.
///jon
On 11/24/22 21:32, Jon Maloy wrote:
Fixes: CVE-2019-14560
GetEfiGlobalVariable2() is used in some instances when looking up the
SecureBoot UEFI variable. The API can fail in certain circumstances,
for example, if AllocatePool() fails or if gRT->GetVariable() fails.
In the case of secure boot checks, it is critical that this return value
is checked. if an attacker can cause the API to fail, it would currently
constitute a secure boot bypass.
This return value check is missing in the function
DxeImageVerificationHandler(),
so we add it here.
This commit is almost identical to one suggested by Jian J Wang
on 2019-09-09, but that one was for some reason never posted to the edk2-devel
list. We now make a new attempt to get it reviewed and applied.
Signed-off-by: Jon Maloy
---
.../DxeImageVerificationLib.c | 39 +++
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git
a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 66e2f5eaa3..6c58b71d37 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1686,6 +1686,7 @@ DxeImageVerificationHandler (
RETURN_STATUS PeCoffStatus;
EFI_STATUSHashStatus;
EFI_STATUSDbStatus;
+ EFI_STATUSSecBootStatus;
BOOLEAN IsFound;
SignatureList = NULL;
@@ -1742,23 +1743,29 @@ DxeImageVerificationHandler (
CpuDeadLoop ();
}
- GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID **), NULL);
- //
- // Skip verification if SecureBoot variable doesn't exist.
- //
- if (SecureBoot == NULL) {
-return EFI_SUCCESS;
- }
-
- //
- // Skip verification if SecureBoot is disabled but not AuditMode
- //
- if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
-FreePool (SecureBoot);
-return EFI_SUCCESS;
- }
+ SecBootStatus = GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID
**), NULL);
+ if (!EFI_ERROR (SecBootStatus)) {
+if (SecureBoot == NULL) {
+ //
+ // Skip verification if SecureBoot variable doesn't exist.
+ //
+ return EFI_SUCCESS;
+} else {
+ //
+ // Skip verification if SecureBoot is disabled but not AuditMode
+ //
+ if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
+FreePool (SecureBoot);
+return EFI_SUCCESS;
+ }
+ FreePool (SecureBoot);
+}
+ } else {
+//
+// Assume SecureBoot enabled in the case of error.
+//
+ }
- FreePool (SecureBoot);
//
// Read the Dos header.
[PATCH] SecurityPkg: check return value of GetEfiGlobalVariable2() in DxeImageVerificationHandler()
Fixes: CVE-2019-14560
GetEfiGlobalVariable2() is used in some instances when looking up the
SecureBoot UEFI variable. The API can fail in certain circumstances,
for example, if AllocatePool() fails or if gRT->GetVariable() fails.
In the case of secure boot checks, it is critical that this return value
is checked. if an attacker can cause the API to fail, it would currently
constitute a secure boot bypass.
This return value check is missing in the function
DxeImageVerificationHandler(),
so we add it here.
This commit is almost identical to one suggested by Jian J Wang
on 2019-09-09, but that one was for some reason never posted to the edk2-devel
list. We now make a new attempt to get it reviewed and applied.
Signed-off-by: Jon Maloy
---
.../DxeImageVerificationLib.c | 39 +++
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git
a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 66e2f5eaa3..6c58b71d37 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1686,6 +1686,7 @@ DxeImageVerificationHandler (
RETURN_STATUS PeCoffStatus;
EFI_STATUSHashStatus;
EFI_STATUSDbStatus;
+ EFI_STATUSSecBootStatus;
BOOLEAN IsFound;
SignatureList = NULL;
@@ -1742,23 +1743,29 @@ DxeImageVerificationHandler (
CpuDeadLoop ();
}
- GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID **),
NULL);
- //
- // Skip verification if SecureBoot variable doesn't exist.
- //
- if (SecureBoot == NULL) {
-return EFI_SUCCESS;
- }
-
- //
- // Skip verification if SecureBoot is disabled but not AuditMode
- //
- if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
-FreePool (SecureBoot);
-return EFI_SUCCESS;
- }
+ SecBootStatus = GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID
**), NULL);
+ if (!EFI_ERROR (SecBootStatus)) {
+if (SecureBoot == NULL) {
+ //
+ // Skip verification if SecureBoot variable doesn't exist.
+ //
+ return EFI_SUCCESS;
+} else {
+ //
+ // Skip verification if SecureBoot is disabled but not AuditMode
+ //
+ if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
+FreePool (SecureBoot);
+return EFI_SUCCESS;
+ }
+ FreePool (SecureBoot);
+}
+ } else {
+//
+// Assume SecureBoot enabled in the case of error.
+//
+ }
- FreePool (SecureBoot);
//
// Read the Dos header.
--
2.35.3
[PATCH] hw/usb: add configuration flags for emulated and passthru usb smartcard
We add two new configuration flags, USB_SMARTCARD_PASSTHRU and
USB_SMARTCARD_EMULATED in order to improve configurability
of these two functionalities.
Signed-off-by: Jon Maloy
---
hw/usb/Kconfig | 12
hw/usb/meson.build | 4 ++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index ce4f433976..50a82badd6 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -108,6 +108,18 @@ config USB_SMARTCARD
bool
default y
depends on USB
+select USB_SMARTCARD_PASSTHRU
+select USB_SMARTCARD_EMULATED
+
+config USB_SMARTCARD_PASSTHRU
+bool
+default y
+depends on USB
+
+config USB_SMARTCARD_EMULATED
+bool
+default y
+depends on USB
config USB_STORAGE_MTP
bool
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index 793df42e21..353006fb6c 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -51,8 +51,8 @@ softmmu_ss.add(when: 'CONFIG_USB_SMARTCARD', if_true:
files('dev-smartcard-reade
if cacard.found()
usbsmartcard_ss = ss.source_set()
- usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD',
- if_true: [cacard, files('ccid-card-emulated.c',
'ccid-card-passthru.c')])
+ usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD_EMULATED', if_true: [cacard,
files('ccid-card-emulated.c')])
+ usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD_PASSTHRU', if_true: [cacard,
files('ccid-card-passthru.c')])
hw_usb_modules += {'smartcard': usbsmartcard_ss}
endif
--
2.35.3
Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507
On 5/3/22 05:59, Kevin Wolf wrote: Am 23.03.2022 um 03:25 hat John Snow geschrieben: On Fri, Mar 18, 2022 at 2:50 PM Thomas Huth wrote: On 10/03/2022 18.53, Jon Maloy wrote: On 3/10/22 12:14, Thomas Huth wrote: On 06/02/2022 20.19, Jon Maloy wrote: Trying again with correct email address. ///jon On 2/6/22 14:15, Jon Maloy wrote: On 1/27/22 15:14, Jon Maloy wrote: On 11/18/21 06:57, Philippe Mathieu-Daudé wrote: Trivial fix for CVE-2021-3507. Philippe Mathieu-Daudé (2): hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507) tests/qtest/fdc-test: Add a regression test for CVE-2021-3507 hw/block/fdc.c | 8 tests/qtest/fdc-test.c | 20 2 files changed, 28 insertions(+) Series Acked-by: Jon Maloy Philippe, I hear from other sources that you earlier have qualified this one as "incomplete". I am of course aware that this one, just like my own patch, is just a mitigation and not a complete correction of the erroneous calculation. Or did you have anything else in mind? Any news on this one? It would be nice to get the CVE fixed for 7.0 ? Thomas The ball is currently with John Snow, as I understand it. The concern is that this fix may not take the driver back to a consistent state, so that we may have other problems later. Maybe Philippe can chip in with a comment here? John, Philippe, any ideas how to move this forward? Thomas The ball is indeed in my court. I need to audit this properly and get the patch re-applied, and get tests passing. As a personal favor: Could you please ping me on IRC tomorrow about this? (Well, later today, for you.) Going through old patches... Is this one still open? Kevin Yes, it is. ///jon
Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507
On 3/10/22 12:14, Thomas Huth wrote: On 06/02/2022 20.19, Jon Maloy wrote: Trying again with correct email address. ///jon On 2/6/22 14:15, Jon Maloy wrote: On 1/27/22 15:14, Jon Maloy wrote: On 11/18/21 06:57, Philippe Mathieu-Daudé wrote: Trivial fix for CVE-2021-3507. Philippe Mathieu-Daudé (2): hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507) tests/qtest/fdc-test: Add a regression test for CVE-2021-3507 hw/block/fdc.c | 8 tests/qtest/fdc-test.c | 20 2 files changed, 28 insertions(+) Series Acked-by: Jon Maloy Philippe, I hear from other sources that you earlier have qualified this one as "incomplete". I am of course aware that this one, just like my own patch, is just a mitigation and not a complete correction of the erroneous calculation. Or did you have anything else in mind? Any news on this one? It would be nice to get the CVE fixed for 7.0 ? Thomas The ball is currently with John Snow, as I understand it. The concern is that this fix may not take the driver back to a consistent state, so that we may have other problems later. Maybe Philippe can chip in with a comment here? ///jon
Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507
Trying again with correct email address. ///jon On 2/6/22 14:15, Jon Maloy wrote: On 1/27/22 15:14, Jon Maloy wrote: On 11/18/21 06:57, Philippe Mathieu-Daudé wrote: Trivial fix for CVE-2021-3507. Philippe Mathieu-Daudé (2): hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507) tests/qtest/fdc-test: Add a regression test for CVE-2021-3507 hw/block/fdc.c | 8 tests/qtest/fdc-test.c | 20 2 files changed, 28 insertions(+) Series Acked-by: Jon Maloy Philippe, I hear from other sources that you earlier have qualified this one as "incomplete". I am of course aware that this one, just like my own patch, is just a mitigation and not a complete correction of the erroneous calculation. Or did you have anything else in mind? Regards ///jon
Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507
On 1/27/22 15:14, Jon Maloy wrote: On 11/18/21 06:57, Philippe Mathieu-Daudé wrote: Trivial fix for CVE-2021-3507. Philippe Mathieu-Daudé (2): hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507) tests/qtest/fdc-test: Add a regression test for CVE-2021-3507 hw/block/fdc.c | 8 tests/qtest/fdc-test.c | 20 2 files changed, 28 insertions(+) Series Acked-by: Jon Maloy Philippe, I hear from other sources that you earlier have qualified this one as "incomplete". I am of course aware that this one, just like my own patch, is just a mitigation and not a complete correction of the erroneous calculation. Or did you have anything else in mind? Regards ///jon
Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507
On 11/18/21 06:57, Philippe Mathieu-Daudé wrote: Trivial fix for CVE-2021-3507. Philippe Mathieu-Daudé (2): hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507) tests/qtest/fdc-test: Add a regression test for CVE-2021-3507 hw/block/fdc.c | 8 tests/qtest/fdc-test.c | 20 2 files changed, 28 insertions(+) Series Acked-by: Jon Maloy
Re: [PATCH] fdc: check for illegal dma length calculation
On 1/13/22 20:33, Jon Maloy wrote:
The function fdctrl_start_transfer() calculates the dma data length
wrongly when certain boundary conditions are fulfilled. We have
noticed that the if ((fdctrl->fifo[5] - fdctrl->fifo[6]) > 1) we get
a dma length that will be interpreted as negative by the next function
in the chain, fdctrl_transfer_handler(). This leads to a crash.
Rather than trying to fix this obscure calculation, we just check if
the harmful condition is fulfilled, and return without action if that
is the case. Since this is a condition that can only be created by a
malicious user we deem this solution safe.
This fix is intended to address CVE-2021-3507.
Signed-off-by: Jon Maloy
---
hw/block/fdc.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 21d18ac2e3..80a1f1750a 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1532,6 +1532,11 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int
direction)
if (fdctrl->fifo[0] & 0x80)
tmp += fdctrl->fifo[6];
fdctrl->data_len *= tmp;
+if (tmp < 0) {
+FLOPPY_DPRINTF("calculated illegal data_len=%u, tmp=%i\n",
+ fdctrl->data_len, tmp);
+return;
+}
}
fdctrl->eot = fdctrl->fifo[6];
if (fdctrl->dor & FD_DOR_DMAEN) {
I never received any feedback on this one.
Is there any?
///jon
[PATCH] fdc: check for illegal dma length calculation
The function fdctrl_start_transfer() calculates the dma data length
wrongly when certain boundary conditions are fulfilled. We have
noticed that the if ((fdctrl->fifo[5] - fdctrl->fifo[6]) > 1) we get
a dma length that will be interpreted as negative by the next function
in the chain, fdctrl_transfer_handler(). This leads to a crash.
Rather than trying to fix this obscure calculation, we just check if
the harmful condition is fulfilled, and return without action if that
is the case. Since this is a condition that can only be created by a
malicious user we deem this solution safe.
This fix is intended to address CVE-2021-3507.
Signed-off-by: Jon Maloy
---
hw/block/fdc.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 21d18ac2e3..80a1f1750a 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1532,6 +1532,11 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int
direction)
if (fdctrl->fifo[0] & 0x80)
tmp += fdctrl->fifo[6];
fdctrl->data_len *= tmp;
+if (tmp < 0) {
+FLOPPY_DPRINTF("calculated illegal data_len=%u, tmp=%i\n",
+ fdctrl->data_len, tmp);
+return;
+}
}
fdctrl->eot = fdctrl->fifo[6];
if (fdctrl->dor & FD_DOR_DMAEN) {
--
2.31.1
Re: [PATCH] e1000: fix tx re-entrancy problem
This was the one I received. ///jon On 12/16/21 14:01, Alexander Bulekov wrote: On 211216 1935, Philippe Mathieu-Daudé wrote: On 12/16/21 16:51, Jon Maloy wrote: On 12/16/21 04:36, Philippe Mathieu-Daudé wrote: Hi Jon, On 10/21/21 18:10, Jon Maloy wrote: The fact that the MMIO handler is not re-entrant causes an infinite loop under certain conditions: Guest write to TDT -> Loopback -> RX (DMA to TDT) -> TX We now eliminate the effect of this problem locally in e1000, by adding a boolean in struct E1000State indicating when the TX side is busy. This will cause any entering new call to return early instead of interfering with the ongoing work, and eliminates any risk of looping. This is intended to address CVE-2021-20257. Signed-off-by: Jon Maloy --- hw/net/e1000.c | 7 +++ 1 file changed, 7 insertions(+) I can not find the reproducer in the repository, have you sent one? No, I did not add it to the repo. It was referenced from the tracker BZ, but I was unable to get access back then. It ended up with that I had it sent by mail to me directly. What is your question? Is it that it should be in the repo, or that you cannot find it? Well I'd like to reproduce the bug, but first I can not find it ;) Having such reproducer committed along with the fix help catching future regressions if we refactor code elsewhere. Blind guess, but assuming write to TDT == set_tctl, maybe this one? https://bugs.launchpad.net/qemu/+bug/1917082 #!/bin/sh cat << EOF > inp clock_step clock_step clock_step clock_step clock_step clock_step outl 0xcf8 0x8813 outl 0xcfc 0xfffe outl 0xcf8 0x8803 outw 0xcfc 0x66e2 write 0xfe000102 0x1 0xff clock_step writel 0xfe20 0x420ff00 write 0xfe00280a 0x3 0x2828ff clock_step clock_step clock_step write 0xfe002815 0x4 0x0300ff46 clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step write 0xf27 0x1 0xff write 0xf98 0x1 0xd5 write 0xf99 0x1 0xd5 write 0xf9b 0x1 0xd5 write 0x1060 0x1 0x17 write 0x1061 0x1 0x38 write 0x1062 0x3 0x00fe00 writel 0xfe0003ff 0x8e8e8e8e write 0xfe00380a 0x3 0x525e03 write 0xfe003818 0x1 0xff EOF ./x86_64-softmmu/qemu-system-x86_64 -display none -machine accel=qtest \ -m 512M -M q35 -nodefaults -device e1000,netdev=net0 \ -netdev user,id=net0 -qtest-log none -qtest stdio < inp
Re: [PATCH] e1000: fix tx re-entrancy problem
On 12/16/21 04:36, Philippe Mathieu-Daudé wrote: Hi Jon, On 10/21/21 18:10, Jon Maloy wrote: The fact that the MMIO handler is not re-entrant causes an infinite loop under certain conditions: Guest write to TDT -> Loopback -> RX (DMA to TDT) -> TX We now eliminate the effect of this problem locally in e1000, by adding a boolean in struct E1000State indicating when the TX side is busy. This will cause any entering new call to return early instead of interfering with the ongoing work, and eliminates any risk of looping. This is intended to address CVE-2021-20257. Signed-off-by: Jon Maloy --- hw/net/e1000.c | 7 +++ 1 file changed, 7 insertions(+) I can not find the reproducer in the repository, have you sent one? No, I did not add it to the repo. It was referenced from the tracker BZ, but I was unable to get access back then. It ended up with that I had it sent by mail to me directly. What is your question? Is it that it should be in the repo, or that you cannot find it? ///jon
[PATCH] e1000: fix tx re-entrancy problem
The fact that the MMIO handler is not re-entrant causes an infinite
loop under certain conditions:
Guest write to TDT -> Loopback -> RX (DMA to TDT) -> TX
We now eliminate the effect of this problem locally in e1000, by adding
a boolean in struct E1000State indicating when the TX side is busy. This
will cause any entering new call to return early instead of interfering
with the ongoing work, and eliminates any risk of looping.
This is intended to address CVE-2021-20257.
Signed-off-by: Jon Maloy
---
hw/net/e1000.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index a30546c5d5..f5bc81296d 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -107,6 +107,7 @@ struct E1000State_st {
e1000x_txd_props props;
e1000x_txd_props tso_props;
uint16_t tso_frames;
+bool busy;
} tx;
struct {
@@ -763,6 +764,11 @@ start_xmit(E1000State *s)
return;
}
+if (s->tx.busy) {
+return;
+}
+s->tx.busy = true;
+
while (s->mac_reg[TDH] != s->mac_reg[TDT]) {
base = tx_desc_base(s) +
sizeof(struct e1000_tx_desc) * s->mac_reg[TDH];
@@ -789,6 +795,7 @@ start_xmit(E1000State *s)
break;
}
}
+s->tx.busy = false;
set_ics(s, 0, cause);
}
--
2.31.1
Problems with building using meson and fuzzing
Alex, Stefan & al I am trying to build and run Marc-Andrés Lureaus libslirp code with fuzzing activated, but I am running into build issues. https://gitlab.freedesktop.org/elmarco/libslirp/-/commit/9fba8af484ec6bc10b22e3f49d9e34d95c28b086 Since I am new to meson I thought it might be quicker to ask somebody instead of spending a lot of time on this myself. 1: I cloned the repository. 2: I made a "regular" build as you suggested in the mommit log, using gcc, and made a run. No problem. 3: I then try to build using clang and with fuzzing activated, also as suggested in the commit log: [jmaloy@f31 libslirp]$ CFLAGS="-fsanitize=fuzzer" CC=clang CXX=clang++ meson build-clang -Db_lundef=false (*** Note that the build directory is missing in the commit log's example command) The Meson build system Version: 0.55.3 Source dir: /home/jmaloy/fuzzing/lureau/libslirp Build dir: /home/jmaloy/fuzzing/lureau/libslirp/build-clang Build type: native build Project name: slirp Project version: 4.0.0 Using 'CC' from environment with value: 'clang' Using 'CFLAGS' from environment with value: '-fsanitize=fuzzer' meson.build:1:0: ERROR: Compiler clang can not compile programs. A full log can be found at /home/jmaloy/fuzzing/lureau/libslirp/build-clang/meson-logs/meson-log.txt [jmaloy@f31 libslirp]$ The content of the indicated log file: Build started at 2020-09-17T19:04:23.217850 Main binary: /usr/bin/python3 Build Options: -Db_lundef=false Python system: Linux The Meson build system Version: 0.55.3 Source dir: /home/jmaloy/fuzzing/lureau/libslirp Build dir: /home/jmaloy/fuzzing/lureau/libslirp/build-clang Build type: native build None of 'PKG_CONFIG_PATH' are defined in the environment, not changing global flags. None of 'PKG_CONFIG_PATH' are defined in the environment, not changing global flags. Project name: slirp Project version: 4.0.0 Using 'CC' from environment with value: 'clang' Using 'CFLAGS' from environment with value: '-fsanitize=fuzzer' None of 'LDFLAGS' are defined in the environment, not changing global flags. None of 'CPPFLAGS' are defined in the environment, not changing global flags. None of 'CC_LD' are defined in the environment, not changing global flags. Sanity testing C compiler: clang Is cross compiler: False. None of 'CC_LD' are defined in the environment, not changing global flags. Sanity check compiler command line: clang /home/jmaloy/fuzzing/lureau/libslirp/build-clang/meson-private/sanitycheckc.c -o /home/jmaloy/fuzzing/lureau/libslirp/build-clang/meson-private/sanitycheckc.exe -fsanitize=fuzzer -pipe -D_FILE_OFFSET_BITS=64 Sanity check compile stdout: - Sanity check compile stderr: /usr/bin/ld: /tmp/sanitycheckc-689218.o: in function `main': sanitycheckc.c:(.text.main[main]+0x0): multiple definition of `main'; /usr/lib64/clang/9.0.1/lib/linux/libclang_rt.fuzzer-x86_64.a(FuzzerMain.cpp.o):(.text.startup[.text.startup.group]+0x0): first defined here /usr/bin/ld: /usr/lib64/clang/9.0.1/lib/linux/libclang_rt.fuzzer-x86_64.a(FuzzerMain.cpp.o): in function `main': (.text.startup[.text.startup.group]+0xf): undefined reference to `LLVMFuzzerTestOneInput' clang-9: error: linker command failed with exit code 1 (use -v to see invocation) - meson.build:1:0: ERROR: Compiler clang can not compile programs. My environment: -- [jmaloy@f31 libslirp]$ which clang /usr/bin/clang [jmaloy@f31 libslirp]$ clang --version clang version 9.0.1 (Fedora 9.0.1-2.fc31) Target: x86_64-unknown-linux-gnu Thread model: posix InstalledDir: /usr/bin [jmaloy@f31 libslirp]$ which meson ~/.local/bin/meson [jmaloy@f31 libslirp]$ meson --version 0.55.3 [jmaloy@f31 libslirp]$ I updated from meson 0.52.0 to 0.55.3, but the result is exactly the same. I commented out "main()" in fuzz-main.c just to check, but that is not the issue of course. To me it looks like the sanity checker is trying to link to libclang_rt.fuzzer-x86_64.a twice, and at the same time is incapable of finding LLVMFuzzerTestOneInput() which clearly is there. Does anybody have any ideas about this? BR ///Jon Maloy
