| Message ID | 1613447214-81951-6-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409 | expand |
On 2/16/21 4:46 AM, Bin Meng wrote: > The codes to limit the maximum block size is only necessary when > SDHC_BLKSIZE register is writable. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > Changes in v2: > - new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable > > hw/sd/sdhci.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 2/18/21 6:09 PM, Philippe Mathieu-Daudé wrote: > On 2/16/21 4:46 AM, Bin Meng wrote: >> The codes to limit the maximum block size is only necessary when >> SDHC_BLKSIZE register is writable. Per "SD Command Generation": The Host Driver should not read the SDMA System Address, Block Size and Block Count registers during a data transaction unless the transfer is stopped because the value is changing and not stable. To prevent destruction of registers using data transfer when issuing command, the 32-bit Block Count, Block Size, 16-bit Block Count and Transfer Mode registers shall be write protected by the Host Controller while Command Inhibit (DAT) is set to 1 in the Present State register. Shouldn't we check for !(s->prnsts & SDHC_DATA_INHIBIT) instead? >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> >> --- >> >> Changes in v2: >> - new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable >> >> hw/sd/sdhci.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >
Hi Philippe, On Fri, Feb 19, 2021 at 2:03 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 2/18/21 6:09 PM, Philippe Mathieu-Daudé wrote: > > On 2/16/21 4:46 AM, Bin Meng wrote: > >> The codes to limit the maximum block size is only necessary when > >> SDHC_BLKSIZE register is writable. > > Per "SD Command Generation": > > The Host Driver should not read the SDMA System Address, Block Size > and Block Count registers during a data transaction unless the > transfer is stopped because the value is changing and not stable. > To prevent destruction of registers using data transfer when issuing > command, the 32-bit Block Count, Block Size, 16-bit Block Count and > Transfer Mode registers shall be write protected by the Host > Controller while Command Inhibit (DAT) is set to 1 in the Present > State register. > > Shouldn't we check for !(s->prnsts & SDHC_DATA_INHIBIT) instead? Yes, for accurate emulation I think we should. Current implementation uses !(s->prnsts & (SDHC_DOING_READ | SDHC_DOING_WRITE)) which eventually is correct, because: SDHC_DATA_INHIBIT bit is set if either SDHC_DAT_LINE_ACTIVE or SDHC_DOING_READ is set (SD Host Controller Spec v7.00 chapter 2.2.9 Present State Register) SDHC_DAT_LINE_ACTIVE bit is set after the end bit of read or write command, and after end bit of read or write command will generate SDHC_DOING_READ or SDHC_DOING_WRITE (SD Host Controller Spec v7.00 chapter 2.2.9 Present State Register) Regards, Bin
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 7a2003b..d0c8e29 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1142,15 +1142,15 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) if (!TRANSFERRING_DATA(s->prnsts)) { MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12)); MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16); - } - /* Limit block size to the maximum buffer size */ - if (extract32(s->blksize, 0, 12) > s->buf_maxsz) { - qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than " - "the maximum buffer 0x%x\n", __func__, s->blksize, - s->buf_maxsz); + /* Limit block size to the maximum buffer size */ + if (extract32(s->blksize, 0, 12) > s->buf_maxsz) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than " + "the maximum buffer 0x%x\n", __func__, s->blksize, + s->buf_maxsz); - s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz); + s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz); + } } break;
The codes to limit the maximum block size is only necessary when SDHC_BLKSIZE register is writable. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- Changes in v2: - new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable hw/sd/sdhci.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)