Re: [PATCH 1/2] hw/ide/core.c (cmd_read_native_max): Avoid limited device parameters
On 230112 0412, Lev Kujawski wrote: > > John Snow writes: > > > On Mon, Oct 10, 2022 at 4:52 AM Lev Kujawski wrote: > >> > >> Always use the native CHS device parameters for the ATA commands READ > >> NATIVE MAX ADDRESS and READ NATIVE MAX ADDRESS EXT, not those limited > >> by the ATA command INITIALIZE_DEVICE_PARAMETERS (introduced in patch > >> 176e4961, hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS > >> command, 2022-07-07.) > >> > >> As stated by the ATA/ATAPI specification, "[t]he native maximum is the > >> highest address accepted by the device in the factory default > >> condition." Therefore this patch substitutes the native values in > >> drive_heads and drive_sectors before calling ide_set_sector(). > >> > >> One consequence of the prior behavior was that setting zero sectors > >> per track could lead to an FPE within ide_set_sector(). Thanks to > >> Alexander Bulekov for reporting this issue. > >> > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1243 > >> Signed-off-by: Lev Kujawski > > > > Does this need attention? > > > > --js > > > > Hi John, > > This patch needs to be merged to mitigate issue 1243, which is still > present within QEMU master as of aa96ab7c9d. > > Thanks, Lev > Ping. oss-fuzz re-discovered this bug.
Re: [PATCH 1/2] hw/ide/core.c (cmd_read_native_max): Avoid limited device parameters
John Snow writes: > On Mon, Oct 10, 2022 at 4:52 AM Lev Kujawski wrote: >> >> Always use the native CHS device parameters for the ATA commands READ >> NATIVE MAX ADDRESS and READ NATIVE MAX ADDRESS EXT, not those limited >> by the ATA command INITIALIZE_DEVICE_PARAMETERS (introduced in patch >> 176e4961, hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS >> command, 2022-07-07.) >> >> As stated by the ATA/ATAPI specification, "[t]he native maximum is the >> highest address accepted by the device in the factory default >> condition." Therefore this patch substitutes the native values in >> drive_heads and drive_sectors before calling ide_set_sector(). >> >> One consequence of the prior behavior was that setting zero sectors >> per track could lead to an FPE within ide_set_sector(). Thanks to >> Alexander Bulekov for reporting this issue. >> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1243 >> Signed-off-by: Lev Kujawski > > Does this need attention? > > --js > Hi John, This patch needs to be merged to mitigate issue 1243, which is still present within QEMU master as of aa96ab7c9d. Thanks, Lev
Re: [PATCH 1/2] hw/ide/core.c (cmd_read_native_max): Avoid limited device parameters
On Mon, Oct 10, 2022 at 4:52 AM Lev Kujawski wrote:
>
> Always use the native CHS device parameters for the ATA commands READ
> NATIVE MAX ADDRESS and READ NATIVE MAX ADDRESS EXT, not those limited
> by the ATA command INITIALIZE_DEVICE_PARAMETERS (introduced in patch
> 176e4961, hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS
> command, 2022-07-07.)
>
> As stated by the ATA/ATAPI specification, "[t]he native maximum is the
> highest address accepted by the device in the factory default
> condition." Therefore this patch substitutes the native values in
> drive_heads and drive_sectors before calling ide_set_sector().
>
> One consequence of the prior behavior was that setting zero sectors
> per track could lead to an FPE within ide_set_sector(). Thanks to
> Alexander Bulekov for reporting this issue.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1243
> Signed-off-by: Lev Kujawski
Does this need attention?
--js
> ---
> hw/ide/core.c | 21 +
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 39afdc0006..ee836401bc 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1608,11 +1608,24 @@ static bool cmd_read_native_max(IDEState *s, uint8_t
> cmd)
> /* Refuse if no sectors are addressable (e.g. medium not inserted) */
> if (s->nb_sectors == 0) {
> ide_abort_command(s);
> -return true;
> -}
> +} else {
> +/*
> + * Save the active drive parameters, which may have been
> + * limited from their native counterparts by, e.g., INITIALIZE
> + * DEVICE PARAMETERS or SET MAX ADDRESS.
> + */
> +const int aheads = s->heads;
> +const int asectors = s->sectors;
>
> -ide_cmd_lba48_transform(s, lba48);
> -ide_set_sector(s, s->nb_sectors - 1);
> +s->heads = s->drive_heads;
> +s->sectors = s->drive_sectors;
> +
> +ide_cmd_lba48_transform(s, lba48);
> +ide_set_sector(s, s->nb_sectors - 1);
> +
> +s->heads = aheads;
> +s->sectors = asectors;
> +}
>
> return true;
> }
> --
> 2.34.1
>
[PATCH 1/2] hw/ide/core.c (cmd_read_native_max): Avoid limited device parameters
Always use the native CHS device parameters for the ATA commands READ
NATIVE MAX ADDRESS and READ NATIVE MAX ADDRESS EXT, not those limited
by the ATA command INITIALIZE_DEVICE_PARAMETERS (introduced in patch
176e4961, hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS
command, 2022-07-07.)
As stated by the ATA/ATAPI specification, "[t]he native maximum is the
highest address accepted by the device in the factory default
condition." Therefore this patch substitutes the native values in
drive_heads and drive_sectors before calling ide_set_sector().
One consequence of the prior behavior was that setting zero sectors
per track could lead to an FPE within ide_set_sector(). Thanks to
Alexander Bulekov for reporting this issue.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1243
Signed-off-by: Lev Kujawski
---
hw/ide/core.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 39afdc0006..ee836401bc 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1608,11 +1608,24 @@ static bool cmd_read_native_max(IDEState *s, uint8_t
cmd)
/* Refuse if no sectors are addressable (e.g. medium not inserted) */
if (s->nb_sectors == 0) {
ide_abort_command(s);
-return true;
-}
+} else {
+/*
+ * Save the active drive parameters, which may have been
+ * limited from their native counterparts by, e.g., INITIALIZE
+ * DEVICE PARAMETERS or SET MAX ADDRESS.
+ */
+const int aheads = s->heads;
+const int asectors = s->sectors;
-ide_cmd_lba48_transform(s, lba48);
-ide_set_sector(s, s->nb_sectors - 1);
+s->heads = s->drive_heads;
+s->sectors = s->drive_sectors;
+
+ide_cmd_lba48_transform(s, lba48);
+ide_set_sector(s, s->nb_sectors - 1);
+
+s->heads = aheads;
+s->sectors = asectors;
+}
return true;
}
--
2.34.1
