| Message ID | 1365155461-11072-1-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
Stefan Hajnoczi <stefanha@redhat.com> writes: > What is the highest addressable sector on an empty CD-ROM? Nothing is > addressable so produce an error. > > This patch prevents a divide-by-zero in ide_set_sector() since > s->sectors and s->heads would be 0. Not to mention that a sector=-1 > argument would be nonsense. > > Note that WIN_READ_NATIVE_MAX can be triggered using hdparm -N 1024 > /dev/cdrom. The LBA bit will be set to 1 though, so the only easy way > to go down the ide_set_sector() CHS code path which divides by zero is > to comment out the s->select & 0x40 case for testing. Suggests you did that. Have you tried the reproducer with a physical drive? Does it fail the command when empty, too?
On Fri, Apr 05, 2013 at 12:56:07PM +0200, Markus Armbruster wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > > > What is the highest addressable sector on an empty CD-ROM? Nothing is > > addressable so produce an error. > > > > This patch prevents a divide-by-zero in ide_set_sector() since > > s->sectors and s->heads would be 0. Not to mention that a sector=-1 > > argument would be nonsense. > > > > Note that WIN_READ_NATIVE_MAX can be triggered using hdparm -N 1024 > > /dev/cdrom. The LBA bit will be set to 1 though, so the only easy way > > to go down the ide_set_sector() CHS code path which divides by zero is > > to comment out the s->select & 0x40 case for testing. > > Suggests you did that. > > Have you tried the reproducer with a physical drive? Does it fail the > command when empty, too? Believe it or not, I don't have access to an ATAPI CD-ROM drive. Would you be able to try out hdparm -N 1024 /dev/cdrom? Note that READ NATIVE MAX is optional, real drives may not implement it since it seems geared towards the Host Protected Area feature which makes no sense on CD-ROMs. (The idea is a reserved area on the disk where system data can be stored and the OS will not touch it.) Stefan
Stefan Hajnoczi <stefanha@redhat.com> writes: > On Fri, Apr 05, 2013 at 12:56:07PM +0200, Markus Armbruster wrote: >> Stefan Hajnoczi <stefanha@redhat.com> writes: >> >> > What is the highest addressable sector on an empty CD-ROM? Nothing is >> > addressable so produce an error. >> > >> > This patch prevents a divide-by-zero in ide_set_sector() since >> > s->sectors and s->heads would be 0. Not to mention that a sector=-1 >> > argument would be nonsense. >> > >> > Note that WIN_READ_NATIVE_MAX can be triggered using hdparm -N 1024 >> > /dev/cdrom. The LBA bit will be set to 1 though, so the only easy way >> > to go down the ide_set_sector() CHS code path which divides by zero is >> > to comment out the s->select & 0x40 case for testing. >> >> Suggests you did that. >> >> Have you tried the reproducer with a physical drive? Does it fail the >> command when empty, too? > > Believe it or not, I don't have access to an ATAPI CD-ROM drive. Would > you be able to try out hdparm -N 1024 /dev/cdrom? > > Note that READ NATIVE MAX is optional, real drives may not implement it > since it seems geared towards the Host Protected Area feature which > makes no sense on CD-ROMs. (The idea is a reserved area on the disk > where system data can be stored and the OS will not touch it.) > > Stefan # hdparm -N /dev/cdrom /dev/cdrom: READ_NATIVE_MAX_ADDRESS failed: Input/output error # hdparm -N 1024 /dev/cdrom /dev/cdrom: setting max visible sectors to 1024 (temporary) READ_NATIVE_MAX_ADDRESS failed: Input/output error READ_NATIVE_MAX_ADDRESS failed: Input/output error Same with and without media. If the command makes no sense for CD-ROMs, and generally isn't implemented by them, we should consider not implementing either, by clearing its IDE_CD bit in ide_cmd_table.
On Fri, Apr 5, 2013 at 2:57 PM, Markus Armbruster <armbru@redhat.com> wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > >> On Fri, Apr 05, 2013 at 12:56:07PM +0200, Markus Armbruster wrote: >>> Stefan Hajnoczi <stefanha@redhat.com> writes: >>> >>> > What is the highest addressable sector on an empty CD-ROM? Nothing is >>> > addressable so produce an error. >>> > >>> > This patch prevents a divide-by-zero in ide_set_sector() since >>> > s->sectors and s->heads would be 0. Not to mention that a sector=-1 >>> > argument would be nonsense. >>> > >>> > Note that WIN_READ_NATIVE_MAX can be triggered using hdparm -N 1024 >>> > /dev/cdrom. The LBA bit will be set to 1 though, so the only easy way >>> > to go down the ide_set_sector() CHS code path which divides by zero is >>> > to comment out the s->select & 0x40 case for testing. >>> >>> Suggests you did that. >>> >>> Have you tried the reproducer with a physical drive? Does it fail the >>> command when empty, too? >> >> Believe it or not, I don't have access to an ATAPI CD-ROM drive. Would >> you be able to try out hdparm -N 1024 /dev/cdrom? >> >> Note that READ NATIVE MAX is optional, real drives may not implement it >> since it seems geared towards the Host Protected Area feature which >> makes no sense on CD-ROMs. (The idea is a reserved area on the disk >> where system data can be stored and the OS will not touch it.) >> >> Stefan > > # hdparm -N /dev/cdrom > > /dev/cdrom: > READ_NATIVE_MAX_ADDRESS failed: Input/output error > # hdparm -N 1024 /dev/cdrom > > /dev/cdrom: > setting max visible sectors to 1024 (temporary) > READ_NATIVE_MAX_ADDRESS failed: Input/output error > READ_NATIVE_MAX_ADDRESS failed: Input/output error > > Same with and without media. > > If the command makes no sense for CD-ROMs, and generally isn't > implemented by them, we should consider not implementing either, by > clearing its IDE_CD bit in ide_cmd_table. Thank you! You're getting the same result that we get in the guest. This looks good. There's no harm in supporting READ NATIVE MAX when the CD-ROM is inserted. It's basically another way of finding out the block device size. I also like this fix better than blacklisting the command since it now protects us in case HD or CFA devices ever have nb_sectors = 0 too. In other words, it's more defensive than just fixing CD-ROMs. Stefan
Stefan Hajnoczi <stefanha@gmail.com> writes: > On Fri, Apr 5, 2013 at 2:57 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Stefan Hajnoczi <stefanha@redhat.com> writes: >> >>> On Fri, Apr 05, 2013 at 12:56:07PM +0200, Markus Armbruster wrote: >>>> Stefan Hajnoczi <stefanha@redhat.com> writes: >>>> >>>> > What is the highest addressable sector on an empty CD-ROM? Nothing is >>>> > addressable so produce an error. >>>> > >>>> > This patch prevents a divide-by-zero in ide_set_sector() since >>>> > s->sectors and s->heads would be 0. Not to mention that a sector=-1 >>>> > argument would be nonsense. >>>> > >>>> > Note that WIN_READ_NATIVE_MAX can be triggered using hdparm -N 1024 >>>> > /dev/cdrom. The LBA bit will be set to 1 though, so the only easy way >>>> > to go down the ide_set_sector() CHS code path which divides by zero is >>>> > to comment out the s->select & 0x40 case for testing. >>>> >>>> Suggests you did that. >>>> >>>> Have you tried the reproducer with a physical drive? Does it fail the >>>> command when empty, too? >>> >>> Believe it or not, I don't have access to an ATAPI CD-ROM drive. Would >>> you be able to try out hdparm -N 1024 /dev/cdrom? >>> >>> Note that READ NATIVE MAX is optional, real drives may not implement it >>> since it seems geared towards the Host Protected Area feature which >>> makes no sense on CD-ROMs. (The idea is a reserved area on the disk >>> where system data can be stored and the OS will not touch it.) >>> >>> Stefan >> >> # hdparm -N /dev/cdrom >> >> /dev/cdrom: >> READ_NATIVE_MAX_ADDRESS failed: Input/output error >> # hdparm -N 1024 /dev/cdrom >> >> /dev/cdrom: >> setting max visible sectors to 1024 (temporary) >> READ_NATIVE_MAX_ADDRESS failed: Input/output error >> READ_NATIVE_MAX_ADDRESS failed: Input/output error >> >> Same with and without media. >> >> If the command makes no sense for CD-ROMs, and generally isn't >> implemented by them, we should consider not implementing either, by >> clearing its IDE_CD bit in ide_cmd_table. > > Thank you! > > You're getting the same result that we get in the guest. This looks good. > > There's no harm in supporting READ NATIVE MAX when the CD-ROM is > inserted. It's basically another way of finding out the block device > size. > > I also like this fix better than blacklisting the command since it now > protects us in case HD or CFA devices ever have nb_sectors = 0 too. > In other words, it's more defensive than just fixing CD-ROMs. Good point. I'd probably do both, simply because a command that isn't available is 100% bug-free. But that's in the realm of artistic license. Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Fri, Apr 05, 2013 at 11:51:01AM +0200, Stefan Hajnoczi wrote: > What is the highest addressable sector on an empty CD-ROM? Nothing is > addressable so produce an error. > > This patch prevents a divide-by-zero in ide_set_sector() since > s->sectors and s->heads would be 0. Not to mention that a sector=-1 > argument would be nonsense. > > Note that WIN_READ_NATIVE_MAX can be triggered using hdparm -N 1024 > /dev/cdrom. The LBA bit will be set to 1 though, so the only easy way > to go down the ide_set_sector() CHS code path which divides by zero is > to comment out the s->select & 0x40 case for testing. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/ide/core.c | 4 ++++ > 1 file changed, 4 insertions(+) Applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
diff --git a/hw/ide/core.c b/hw/ide/core.c index 3743dc3..77f1379 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1262,6 +1262,10 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val) lba48 = 1; /* fall through */ case WIN_READ_NATIVE_MAX: + /* Refuse if no sectors are addressable (e.g. medium not inserted) */ + if (s->nb_sectors == 0) { + goto abort_cmd; + } ide_cmd_lba48_transform(s, lba48); ide_set_sector(s, s->nb_sectors - 1); s->status = READY_STAT | SEEK_STAT;
What is the highest addressable sector on an empty CD-ROM? Nothing is addressable so produce an error. This patch prevents a divide-by-zero in ide_set_sector() since s->sectors and s->heads would be 0. Not to mention that a sector=-1 argument would be nonsense. Note that WIN_READ_NATIVE_MAX can be triggered using hdparm -N 1024 /dev/cdrom. The LBA bit will be set to 1 though, so the only easy way to go down the ide_set_sector() CHS code path which divides by zero is to comment out the s->select & 0x40 case for testing. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/ide/core.c | 4 ++++ 1 file changed, 4 insertions(+)