From: John Snow <jsnow@redhat.com>
To: P J P <ppandit@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>,
"hannes Reinecke" <hare@suse.com>, "Gaoning Pan" <pgn@zju.edu.cn>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>,
"QEMU Developers" <qemu-devel@nongnu.org>
Subject: Re: [PATCH] fdc: check drive block device before usage (CVE-2021-20196)
Date: Tue, 18 May 2021 10:18:12 -0400 [thread overview]
Message-ID: <9ff4a630-6b48-70f7-41ee-36368a68782e@redhat.com> (raw)
In-Reply-To: <84950qo-8613-oo49-o876-p1op4r3ss60@erqung.pbz>
On 5/18/21 5:01 AM, P J P wrote:
> Hello John,
>
> +-- On Mon, 17 May 2021, John Snow wrote --+
> | > /* Selected drive */
> | > - fdctrl->cur_drv = value & FD_DOR_SELMASK;
> | > + if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
> | > + fdctrl->cur_drv = value & FD_DOR_SELMASK;
> | > + }
> |
> | I don't think this is correct. If you look at get_cur_drv(), it uses the
> | TDR_BOOTSEL bit to change the logical mappings of "drive 0" or "drive 1" to be
> | reversed. You don't check that bit here, so you might be checking the wrong
> | drive.
> |
> | Plus, the TDR bit can change later, so I think you shouldn't actually protect
> | the register write like this. Just delete this bit of code. We ought to
> | protect the drives when we go to use them instead of preventing the registers
> | from getting "the wrong values".
>
> * I see.
>
(I know this is extremely backwards from how good code ought to be
written where we centralize protecting sane values from becoming object
state.)
> | > cur_drv = get_cur_drv(fdctrl);
> | > + if (!cur_drv->blk) {
> | > + FLOPPY_DPRINTF("No drive connected\n");
> | > + return 0;
> | > + }
> |
> | This seems fine ... or at least not worse than the other error handling we
> | already have here. (Which seems to be ... basically, none. We just ignore the
> | write and do nothing, which seems wrong. I guess it's better than a crash...
> | but I don't have the time to do a proper audit of what this is SUPPOSED to do
> | in this case.)
> |
(Which, to be clear, is 100% OK by me for this patch.)
> | > - if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> | > + if (cur_drv->blk == NULL
> | > + || blk_pwrite(cur_drv->blk, fd_offset(cur_drv),
> | > fdctrl->fifo,
> |
> | Seems fine, but if we had a drive for the earlier check, will we really be in
> | a situation where we don't have one now?
> |
> | Ignore the bit I sent earlier about the qtest reproducer not correlating to
> | this patch -- it does, I was experiencing an unrelated crash.
>
> * Okay.
>
>
> | On 5/17/21 7:12 AM, P J P wrote:
> | > Do we need a revised patch[-series] including a qtest? OR it can be done at
> | > merge time?
> |
> | If you have the time to write a qtest reproducer, you can send it separately
> | and I'll pick it up if everything looks correct.
>
> * Yes, that seems better, I'll try to create a qtest, but it may take time.
>
Understand. Maybe I can help. The fuzzer reproducer is a great first
step, but just needs to be "back-translated" into the logical operations
it is performing so that the test code is readable.
I started doodling a tracer patch similar to the IDE one I checked in
some ages ago to give symbolic names to the registers on read/write,
which makes "reading" Alexander's fuzzing reproducers a bit easier.
I'll go work on that for a little while.
> * I'll check and revise the patch with above details asap.
OK; on your own schedule. I will try to leap on the patches as soon as I
get them before the FDC code falls out of my head again.
If at all possible, I wouldn't mind seeing a series bundled with the
other FDC fixes outstanding aggregated together. It will be easier (for
me) to make sure I have everything up to date and together. (If it isn't
too much hassle for you.)
AFAIK there's one for reads and one for writes that are very similar --
they protect against null BLK reads when we do not have a floppy drive
present.
Thank you,
--js
>
>
> Thank you.
> --
> - P J P
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>
next prev parent reply other threads:[~2021-05-18 14:19 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-23 10:03 [PATCH] fdc: check drive block device before usage (CVE-2021-20196) P J P
2021-01-23 17:47 ` Alexander Bulekov
2021-01-23 17:52 ` Alexander Bulekov
2021-05-17 19:41 ` John Snow
2021-01-30 13:35 ` P J P
2021-05-14 19:23 ` Thomas Huth
2021-05-14 19:26 ` John Snow
2021-05-15 8:49 ` Philippe Mathieu-Daudé
2021-05-17 11:12 ` P J P
2021-05-17 17:14 ` John Snow
2021-05-17 17:56 ` Philippe Mathieu-Daudé
2021-05-17 22:39 ` John Snow
2021-05-18 9:01 ` P J P
2021-05-18 14:18 ` John Snow [this message]
2021-05-18 17:24 ` John Snow
2021-05-19 7:32 ` P J P
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9ff4a630-6b48-70f7-41ee-36368a68782e@redhat.com \
--to=jsnow@redhat.com \
--cc=hare@suse.com \
--cc=kwolf@redhat.com \
--cc=pgn@zju.edu.cn \
--cc=philmd@redhat.com \
--cc=ppandit@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).