Message ID | 20220114013319.348012-1-jmaloy@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | fdc: check for illegal dma length calculation | expand |
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 <jmaloy@redhat.com> > --- > 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
Hi Jon, On 26/1/22 16:44, Jon Maloy wrote: > 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. If a security issue is reproducible (like the ones found by fuzzers), please include the reproducer along. >> 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. Ah, this is similar to: https://lore.kernel.org/qemu-devel/20211118115733.4038610-1-philmd@redhat.com/ which already contains the reproducer... You might want to review it ;) >> Signed-off-by: Jon Maloy <jmaloy@redhat.com> >> --- >> 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? Probably none so far because you forgot to Cc the maintainers: $ ./scripts/get_maintainer.pl -f hw/block/fdc.c John Snow <jsnow@redhat.com> (supporter:Floppy) Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core) Hanna Reitz <hreitz@redhat.com> (supporter:Block layer core) qemu-block@nongnu.org (open list:Floppy) qemu-devel@nongnu.org (open list:All patches CC here) (now Cc'ed).
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) {
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 <jmaloy@redhat.com> --- hw/block/fdc.c | 5 +++++ 1 file changed, 5 insertions(+)