* [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device
@ 2013-04-05 9:51 Stefan Hajnoczi
2013-04-05 10:56 ` Markus Armbruster
2013-04-10 9:08 ` Stefan Hajnoczi
0 siblings, 2 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2013-04-05 9:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
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(+)
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;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device
2013-04-05 9:51 [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device Stefan Hajnoczi
@ 2013-04-05 10:56 ` Markus Armbruster
2013-04-05 12:19 ` Stefan Hajnoczi
2013-04-10 9:08 ` Stefan Hajnoczi
1 sibling, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2013-04-05 10:56 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device
2013-04-05 10:56 ` Markus Armbruster
@ 2013-04-05 12:19 ` Stefan Hajnoczi
2013-04-05 12:57 ` Markus Armbruster
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Hajnoczi @ 2013-04-05 12:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device
2013-04-05 12:19 ` Stefan Hajnoczi
@ 2013-04-05 12:57 ` Markus Armbruster
2013-04-05 14:07 ` Stefan Hajnoczi
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2013-04-05 12:57 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device
2013-04-05 12:57 ` Markus Armbruster
@ 2013-04-05 14:07 ` Stefan Hajnoczi
2013-04-05 15:11 ` Markus Armbruster
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Hajnoczi @ 2013-04-05 14:07 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device
2013-04-05 14:07 ` Stefan Hajnoczi
@ 2013-04-05 15:11 ` Markus Armbruster
0 siblings, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2013-04-05 15:11 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device
2013-04-05 9:51 [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device Stefan Hajnoczi
2013-04-05 10:56 ` Markus Armbruster
@ 2013-04-10 9:08 ` Stefan Hajnoczi
1 sibling, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2013-04-10 9:08 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Markus Armbruster
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-04-10 9:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-05 9:51 [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device Stefan Hajnoczi
2013-04-05 10:56 ` Markus Armbruster
2013-04-05 12:19 ` Stefan Hajnoczi
2013-04-05 12:57 ` Markus Armbruster
2013-04-05 14:07 ` Stefan Hajnoczi
2013-04-05 15:11 ` Markus Armbruster
2013-04-10 9:08 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.