fdc: Clean up and fix command processing

What all callers of fdctrl_reset_fifo() really want to do is to start the command phase, where writes to the data port initiate a new command.

The function doesn't only clear the FIFO, but also sets up the state so that a new command can be received. Rename it to reflect this.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

hw/block/fdc.c +14 -14

index d8a8edd..9f95ace 100644
@@ -324,7 +324,7 @@ static void fd_revalidate(FDrive *drv)
 /* Intel 82078 floppy disk controller emulation          */
 
 static void fdctrl_reset(FDCtrl *fdctrl, int do_irq);
-static void fdctrl_reset_fifo(FDCtrl *fdctrl);
+static void fdctrl_to_command_phase(FDCtrl *fdctrl);
 static int fdctrl_transfer_handler (void *opaque, int nchan,
                                     int dma_pos, int dma_len);
 static void fdctrl_raise_irq(FDCtrl *fdctrl);

@@ -918,7 +918,7 @@ static void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
     fdctrl->data_dir = FD_DIR_WRITE;
     for (i = 0; i < MAX_FD; i++)
         fd_recalibrate(&fdctrl->drives[i]);
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
     if (do_irq) {
         fdctrl->status0 |= FD_SR0_RDYCHG;
         fdctrl_raise_irq(fdctrl);

@@ -1134,8 +1134,8 @@ static uint32_t fdctrl_read_dir(FDCtrl *fdctrl)
     return retval;
 }
 
-/* FIFO state control */
-static void fdctrl_reset_fifo(FDCtrl *fdctrl)
+/* Clear the FIFO and update the state for receiving the next command */
+static void fdctrl_to_command_phase(FDCtrl *fdctrl)
 {
     fdctrl->data_dir = FD_DIR_WRITE;
     fdctrl->data_pos = 0;

@@ -1533,7 +1533,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         if (fdctrl->msr & FD_MSR_NONDMA) {
             fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
         } else {
-            fdctrl_reset_fifo(fdctrl);
+            fdctrl_to_command_phase(fdctrl);
             fdctrl_reset_irq(fdctrl);
         }
     }

@@ -1667,7 +1667,7 @@ static void fdctrl_handle_restore(FDCtrl *fdctrl, int direction)
     fdctrl->config = fdctrl->fifo[11];
     fdctrl->precomp_trk = fdctrl->fifo[12];
     fdctrl->pwrd = fdctrl->fifo[13];
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
 }
 
 static void fdctrl_handle_save(FDCtrl *fdctrl, int direction)

@@ -1746,7 +1746,7 @@ static void fdctrl_handle_specify(FDCtrl *fdctrl, int direction)
     else
         fdctrl->dor |= FD_DOR_DMAEN;
     /* No result back */
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
 }
 
 static void fdctrl_handle_sense_drive_status(FDCtrl *fdctrl, int direction)

@@ -1772,7 +1772,7 @@ static void fdctrl_handle_recalibrate(FDCtrl *fdctrl, int direction)
     SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
     cur_drv = get_cur_drv(fdctrl);
     fd_recalibrate(cur_drv);
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
     /* Raise Interrupt */
     fdctrl->status0 |= FD_SR0_SEEK;
     fdctrl_raise_irq(fdctrl);

@@ -1808,7 +1808,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
 
     SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
     cur_drv = get_cur_drv(fdctrl);
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
     /* The seek command just sends step pulses to the drive and doesn't care if
      * there is a medium inserted of if it's banging the head against the drive.
      */

@@ -1825,7 +1825,7 @@ static void fdctrl_handle_perpendicular_mode(FDCtrl *fdctrl, int direction)
     if (fdctrl->fifo[1] & 0x80)
         cur_drv->perpendicular = fdctrl->fifo[1] & 0x7;
     /* No result back */
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
 }
 
 static void fdctrl_handle_configure(FDCtrl *fdctrl, int direction)

@@ -1833,7 +1833,7 @@ static void fdctrl_handle_configure(FDCtrl *fdctrl, int direction)
     fdctrl->config = fdctrl->fifo[2];
     fdctrl->precomp_trk =  fdctrl->fifo[3];
     /* No result back */
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
 }
 
 static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)

@@ -1846,7 +1846,7 @@ static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)
 static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
 {
     /* No result back */
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
 }
 
 static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direction)

@@ -1864,7 +1864,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
             fdctrl->fifo[3] = 0;
             fdctrl_set_fifo(fdctrl, 4);
         } else {
-            fdctrl_reset_fifo(fdctrl);
+            fdctrl_to_command_phase(fdctrl);
         }
     } else if (fdctrl->data_len > 7) {
         /* ERROR */

@@ -1887,7 +1887,7 @@ static void fdctrl_handle_relative_seek_in(FDCtrl *fdctrl, int direction)
         fd_seek(cur_drv, cur_drv->head,
                 cur_drv->track + fdctrl->fifo[2], cur_drv->sect, 1);
     }
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
     /* Raise Interrupt */
     fdctrl->status0 |= FD_SR0_SEEK;
     fdctrl_raise_irq(fdctrl);

What callers really do with this function is to switch from execution phase (including data transfers) to result phase where the guest can read out one or more status bytes from the FIFO (the number depends on the command).

Rename the function accordingly.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

hw/block/fdc.c +15 -14

index 9f95ace..8c41434 100644
@@ -1142,8 +1142,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
     fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
 }
 
-/* Set FIFO status for the host to read */
-static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len)
+/* Update the state to allow the guest to read out the command status.
+ * @fifo_len is the number of result bytes to be read out. */
+static void fdctrl_to_result_phase(FDCtrl *fdctrl, int fifo_len)
 {
     fdctrl->data_dir = FD_DIR_READ;
     fdctrl->data_len = fifo_len;

@@ -1157,7 +1158,7 @@ static void fdctrl_unimplemented(FDCtrl *fdctrl, int direction)
     qemu_log_mask(LOG_UNIMP, "fdc: unimplemented command 0x%02x\n",
                   fdctrl->fifo[0]);
     fdctrl->fifo[0] = FD_SR0_INVCMD;
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 /* Seek to next sector

@@ -1238,7 +1239,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     fdctrl->msr |= FD_MSR_RQM | FD_MSR_DIO;
     fdctrl->msr &= ~FD_MSR_NONDMA;
 
-    fdctrl_set_fifo(fdctrl, 7);
+    fdctrl_to_result_phase(fdctrl, 7);
     fdctrl_raise_irq(fdctrl);
 }
 

@@ -1606,7 +1607,7 @@ static void fdctrl_handle_lock(FDCtrl *fdctrl, int direction)
 {
     fdctrl->lock = (fdctrl->fifo[0] & 0x80) ? 1 : 0;
     fdctrl->fifo[0] = fdctrl->lock << 4;
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)

@@ -1631,20 +1632,20 @@ static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)
         (cur_drv->perpendicular << 2);
     fdctrl->fifo[8] = fdctrl->config;
     fdctrl->fifo[9] = fdctrl->precomp_trk;
-    fdctrl_set_fifo(fdctrl, 10);
+    fdctrl_to_result_phase(fdctrl, 10);
 }
 
 static void fdctrl_handle_version(FDCtrl *fdctrl, int direction)
 {
     /* Controller's version */
     fdctrl->fifo[0] = fdctrl->version;
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 static void fdctrl_handle_partid(FDCtrl *fdctrl, int direction)
 {
     fdctrl->fifo[0] = 0x41; /* Stepping 1 */
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 static void fdctrl_handle_restore(FDCtrl *fdctrl, int direction)

@@ -1697,7 +1698,7 @@ static void fdctrl_handle_save(FDCtrl *fdctrl, int direction)
     fdctrl->fifo[12] = fdctrl->pwrd;
     fdctrl->fifo[13] = 0;
     fdctrl->fifo[14] = 0;
-    fdctrl_set_fifo(fdctrl, 15);
+    fdctrl_to_result_phase(fdctrl, 15);
 }
 
 static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction)

@@ -1762,7 +1763,7 @@ static void fdctrl_handle_sense_drive_status(FDCtrl *fdctrl, int direction)
         (cur_drv->head << 2) |
         GET_CUR_DRV(fdctrl) |
         0x28;
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 static void fdctrl_handle_recalibrate(FDCtrl *fdctrl, int direction)

@@ -1788,7 +1789,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
         fdctrl->reset_sensei--;
     } else if (!(fdctrl->sra & FD_SRA_INTPEND)) {
         fdctrl->fifo[0] = FD_SR0_INVCMD;
-        fdctrl_set_fifo(fdctrl, 1);
+        fdctrl_to_result_phase(fdctrl, 1);
         return;
     } else {
         fdctrl->fifo[0] =

@@ -1797,7 +1798,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
     }
 
     fdctrl->fifo[1] = cur_drv->track;
-    fdctrl_set_fifo(fdctrl, 2);
+    fdctrl_to_result_phase(fdctrl, 2);
     fdctrl_reset_irq(fdctrl);
     fdctrl->status0 = FD_SR0_RDYCHG;
 }

@@ -1840,7 +1841,7 @@ static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)
 {
     fdctrl->pwrd = fdctrl->fifo[1];
     fdctrl->fifo[0] = fdctrl->fifo[1];
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)

@@ -1862,7 +1863,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
             fdctrl->fifo[0] = fdctrl->fifo[1];
             fdctrl->fifo[2] = 0;
             fdctrl->fifo[3] = 0;
-            fdctrl_set_fifo(fdctrl, 4);
+            fdctrl_to_result_phase(fdctrl, 4);
         } else {
             fdctrl_to_command_phase(fdctrl);
         }

The floppy controller spec describes three different controller phases, which are currently not explicitly modelled in our emulation. Instead, each phase is represented by a combination of flags in registers.

This patch makes explicit in which phase the controller currently is.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

hw/block/fdc.c +0 -0

index 8c41434..4d4868e 100644

The floppy controller spec describes three different controller phases, which are currently not explicitly modelled in our emulation. Instead, each phase is represented by a combination of flags in registers.

This patch makes explicit in which phase the controller currently is.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

hw/block/fdc.c +24 -0

index 8c41434..4d4868e 100644
@@ -495,6 +495,30 @@ enum {
     FD_DIR_DSKCHG   = 0x80,
 };
 
+/*
+ * See chapter 5.0 "Controller phases" of the spec:
+ *
+ * Command phase:
+ * The host writes a command and its parameters into the FIFO. The command
+ * phase is completed when all parameters for the command have been supplied,
+ * and execution phase is entered.
+ *
+ * Execution phase:
+ * Data transfers, either DMA or non-DMA. For non-DMA transfers, the FIFO
+ * contains the payload now, otherwise it's unused. When all bytes of the
+ * required data have been transferred, the state is switched to either result
+ * phase (if the command produces status bytes) or directly back into the
+ * command phase for the next command.
+ *
+ * Result phase:
+ * The host reads out the FIFO, which contains one or more result bytes now.
+ */
+typedef enum FDCtrlPhase {
+    FD_PHASE_COMMAND,
+    FD_PHASE_EXECUTION,
+    FD_PHASE_RESULT,
+} FDCtrlPhase;
+
 #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
 #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
 

Quoted 46 lines · Kevin Wolf

The floppy controller spec describes three different controller phases, which are currently not explicitly modelled in our emulation. Instead, each phase is represented by a combination of flags in registers.

This patch makes explicit in which phase the controller currently is.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

hw/block/fdc.c +24 -0

index 8c41434..4d4868e 100644
@@ -495,6 +495,30 @@ enum {
     FD_DIR_DSKCHG   = 0x80,
 };
 
+/*
+ * See chapter 5.0 "Controller phases" of the spec:
+ *
+ * Command phase:
+ * The host writes a command and its parameters into the FIFO. The command
+ * phase is completed when all parameters for the command have been supplied,
+ * and execution phase is entered.
+ *
+ * Execution phase:
+ * Data transfers, either DMA or non-DMA. For non-DMA transfers, the FIFO
+ * contains the payload now, otherwise it's unused. When all bytes of the
+ * required data have been transferred, the state is switched to either result
+ * phase (if the command produces status bytes) or directly back into the
+ * command phase for the next command.
+ *
+ * Result phase:
+ * The host reads out the FIFO, which contains one or more result bytes now.
+ */
+typedef enum FDCtrlPhase {
+    FD_PHASE_COMMAND,
+    FD_PHASE_EXECUTION,
+    FD_PHASE_RESULT,
+} FDCtrlPhase;
+
 #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
 #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
 

hw/block/fdc.c struct FDCtrl { · line 531

Quoted 5 lines

hw/block/fdc.c +1 -0

@@ -504,6 +528,7 @@ struct FDCtrl {
     /* Controller state */
     QEMUTimer *result_timer;
     int dma_chann;
+    FDCtrlPhase phase;

This is new state -- don't we need to handle it on migration?

In general I'm not a huge fan of caching derived state in device models...

-- PMM

Hmm, I think this is not purely derived state because the flags are not necessarily sufficient for regenerating that state.

Quoted 2 lines

Hmm, I think this is not purely derived state because the flags are not necessarily sufficient for regenerating that state.

Yeah, if there's genuinely an underlying state machine that's not completely visible in registers you need to actually model it. You should probably then model the register bits by calculating them from the state rather than by changing them as you go along in parallel with moving the state machine around.

Am 19.05.2015 um 22:57 hat Peter Maydell geschrieben:

Quoted 5 lines

Yeah, if there's genuinely an underlying state machine that's not completely visible in registers you need to actually model it. You should probably then model the register bits by calculating them from the state rather than by changing them as you go along in parallel with moving the state machine around.

I think the combination of registers is actually enough to reconstruct what state we're in, so it is derived (otherwise I would have fixed a bug that I'm not aware of). Adding logic to derive it in a post-load handler should be good enough.

Quoted 7 lines

Am 19.05.2015 um 22:57 hat Peter Maydell geschrieben:

Quoted 5 lines

Yeah, if there's genuinely an underlying state machine that's not completely visible in registers you need to actually model it. You should probably then model the register bits by calculating them from the state rather than by changing them as you go along in parallel with moving the state machine around.

Quoted 4 lines

I think the combination of registers is actually enough to reconstruct what state we're in, so it is derived (otherwise I would have fixed a bug that I'm not aware of). Adding logic to derive it in a post-load handler should be good enough.

That handles migration, which is good. But I still think that storing the same information in two places in the device state (phase field and the register fields) is error-prone.

If we want to switch to having a phase field we should calculate the relevant register bits on demand based on the phase, rather than keeping both copies of the state in sync manually.

Am 20.05.2015 um 10:06 hat Peter Maydell geschrieben:

Quoted 3 lines

That handles migration, which is good. But I still think that storing the same information in two places in the device state (phase field and the register fields) is error-prone.

That's actually my point. The registers are accessed everywhere in the code, whereas phase transitions are in very few well-defined places (there are exactly four of them, iirc). If they get out of sync, chances are that the bug is in the register value, not in the phase. When we know what phase we're in, we can assert the bits and actually catch such bugs.

Quoted 3 lines

If we want to switch to having a phase field we should calculate the relevant register bits on demand based on the phase, rather than keeping both copies of the state in sync manually.

That doesn't work, unfortunately. Some register bits imply a specific phase (assuming correct code), but you can't derive the exact bits just from the phase.

Quoted 12 lines · Kevin Wolf

Am 20.05.2015 um 10:06 hat Peter Maydell geschrieben:

Quoted 3 lines

That handles migration, which is good. But I still think that storing the same information in two places in the device state (phase field and the register fields) is error-prone.

That's actually my point. The registers are accessed everywhere in the code, whereas phase transitions are in very few well-defined places (there are exactly four of them, iirc). If they get out of sync, chances are that the bug is in the register value, not in the phase. When we know what phase we're in, we can assert the bits and actually catch such bugs.

Quoted 7 lines

Quoted 3 lines

If we want to switch to having a phase field we should calculate the relevant register bits on demand based on the phase, rather than keeping both copies of the state in sync manually.

That doesn't work, unfortunately. Some register bits imply a specific phase (assuming correct code), but you can't derive the exact bits just from the phase.

Having now dug out a copy of the 82078 spec, I agree that the state isn't derivable purely from the register values in the general case. The controller clearly has a state machine internally but it doesn't surface that in the register state except indirectly.

-- PMM

So even if /currently/ we can reconstitute it from the register values, we may eventually be unable to.

post_load will work for now, but I fear the case (in ten years) when someone else cleans up FDC code but fails to realize that the phase is not explicitly migrated.

Quoted 6 lines

So even if /currently/ we can reconstitute it from the register values, we may eventually be unable to.

post_load will work for now, but I fear the case (in ten years) when someone else cleans up FDC code but fails to realize that the phase is not explicitly migrated.

I assumed we would only do the post-load thing if the source for migration doesn't migrate phase (however we phrase that in a vmstate struct).

Am 20.05.2015 um 14:07 hat Peter Maydell geschrieben:

Quoted 11 lines · John Snow

Quoted 6 lines

So even if /currently/ we can reconstitute it from the register values, we may eventually be unable to.

post_load will work for now, but I fear the case (in ten years) when someone else cleans up FDC code but fails to realize that the phase is not explicitly migrated.

I assumed we would only do the post-load thing if the source for migration doesn't migrate phase (however we phrase that in a vmstate struct).

I think if we extend the VMState, then we have two options. I'm not exactly sure how they work in details, but I'll try an educated guess - Juan, please correct me if I'm wrong:

1. We increase version_id and add the new field at the end. This breaks backwards migration; on forward migration the new field would be initialised with 0 and a post_load handler could check the old version_id to calculate the phase from register bits.

2. We add a subsection for the phase, and declare one phase to be the default (most likely the command phase) for which a subsection is not sent. In this case, the destination can't distinguish between a missing subsection because the source was running an old qemu or because it is the default phase. Unclear whether post_load should recalculate the phase or not.

If the above is correct, I'm afraid that the third option - which doesn't address John's (valid) concerns - would be the most reasonable:

3. Don't add any VMState fields now and just do the post_load handler. If we ever extend fdc in a way that makes it impossible to reconstruct the phase from other migrated state, we add a subsection that is only sent in cases where it differs from the reconstructed value.

Kevin

Am 20.05.2015 um 14:07 hat Peter Maydell geschrieben:

Quoted 11 lines · John Snow

Quoted 6 lines

So even if /currently/ we can reconstitute it from the register values, we may eventually be unable to.

post_load will work for now, but I fear the case (in ten years) when someone else cleans up FDC code but fails to realize that the phase is not explicitly migrated.

I assumed we would only do the post-load thing if the source for migration doesn't migrate phase (however we phrase that in a vmstate struct).

I think if we extend the VMState, then we have two options. I'm not exactly sure how they work in details, but I'll try an educated guess - Juan, please correct me if I'm wrong:

1. We increase version_id and add the new field at the end. This breaks backwards migration; on forward migration the new field would be initialised with 0 and a post_load handler could check the old version_id to calculate the phase from register bits.

2. We add a subsection for the phase, and declare one phase to be the default (most likely the command phase) for which a subsection is not sent. In this case, the destination can't distinguish between a missing subsection because the source was running an old qemu or because it is the default phase. Unclear whether post_load should recalculate the phase or not.

If the above is correct, I'm afraid that the third option - which doesn't address John's (valid) concerns - would be the most reasonable:

3. Don't add any VMState fields now and just do the post_load handler. If we ever extend fdc in a way that makes it impossible to reconstruct the phase from other migrated state, we add a subsection that is only sent in cases where it differs from the reconstructed value.

Quoted 22 lines

Am 20.05.2015 um 14:07 hat Peter Maydell geschrieben:

Quoted 11 lines · John Snow

Quoted 6 lines

So even if /currently/ we can reconstitute it from the register values, we may eventually be unable to.

post_load will work for now, but I fear the case (in ten years) when someone else cleans up FDC code but fails to realize that the phase is not explicitly migrated.

I assumed we would only do the post-load thing if the source for migration doesn't migrate phase (however we phrase that in a vmstate struct).

I think if we extend the VMState, then we have two options. I'm not exactly sure how they work in details, but I'll try an educated guess - Juan, please correct me if I'm wrong:

1. We increase version_id and add the new field at the end. This breaks backwards migration; on forward migration the new field would be initialised with 0 and a post_load handler could check the old version_id to calculate the phase from register bits.

Quoted 6 lines

2. We add a subsection for the phase, and declare one phase to be the default (most likely the command phase) for which a subsection is not sent. In this case, the destination can't distinguish between a missing subsection because the source was running an old qemu or because it is the default phase. Unclear whether post_load should recalculate the phase or not.

2b add a subsection, send the subsection always in new qemu, if receiving from an old qemu then calculate the phase from the register fields in post-load

That's like 1 but just using a subsection rather than a new field. (I have a vague recollection that distros doing backports prefer subsections over increment-version-id-and-add-field).

Quoted 8 lines

If the above is correct, I'm afraid that the third option - which doesn't address John's (valid) concerns - would be the most reasonable:

3. Don't add any VMState fields now and just do the post_load handler. If we ever extend fdc in a way that makes it impossible to reconstruct the phase from other migrated state, we add a subsection that is only sent in cases where it differs from the reconstructed value.

I'm definitely against this one. State should always be sent in migration, and not doing that once we've added it to the device struct is a bug.

Am 21.05.2015 um 12:11 hat Peter Maydell geschrieben:

Quoted 33 lines · Kevin Wolf

Quoted 22 lines

Am 20.05.2015 um 14:07 hat Peter Maydell geschrieben:

Quoted 11 lines · John Snow

Quoted 6 lines

So even if /currently/ we can reconstitute it from the register values, we may eventually be unable to.

post_load will work for now, but I fear the case (in ten years) when someone else cleans up FDC code but fails to realize that the phase is not explicitly migrated.

I assumed we would only do the post-load thing if the source for migration doesn't migrate phase (however we phrase that in a vmstate struct).

I think if we extend the VMState, then we have two options. I'm not exactly sure how they work in details, but I'll try an educated guess - Juan, please correct me if I'm wrong:

1. We increase version_id and add the new field at the end. This breaks backwards migration; on forward migration the new field would be initialised with 0 and a post_load handler could check the old version_id to calculate the phase from register bits.

Quoted 6 lines

2. We add a subsection for the phase, and declare one phase to be the default (most likely the command phase) for which a subsection is not sent. In this case, the destination can't distinguish between a missing subsection because the source was running an old qemu or because it is the default phase. Unclear whether post_load should recalculate the phase or not.

2b add a subsection, send the subsection always in new qemu, if receiving from an old qemu then calculate the phase from the register fields in post-load

Quoted 3 lines

That's like 1 but just using a subsection rather than a new field. (I have a vague recollection that distros doing backports prefer subsections over increment-version-id-and-add-field).

Hm, okay. The post_load handler doesn't have the version id then to check. If a subsection isn't present, are its fields initialised as 0? Or what would be the right way to check whether the subsection was there?

Any pointers to device that do something similar?

Quoted 9 lines

Quoted 8 lines

If the above is correct, I'm afraid that the third option - which doesn't address John's (valid) concerns - would be the most reasonable:

3. Don't add any VMState fields now and just do the post_load handler. If we ever extend fdc in a way that makes it impossible to reconstruct the phase from other migrated state, we add a subsection that is only sent in cases where it differs from the reconstructed value.

Quoted 3 lines

I'm definitely against this one. State should always be sent in migration, and not doing that once we've added it to the device struct is a bug.

On second thought, I could actually add that subsection now and write the .needed callback such that it checks whether the reconstructed phase matches the actual one. This condition would always evaluate as false today, but if we ever change things so that the phase doesn't match what current qemu would think it is, you automatically get the subsection and things Just Work (TM).

This also wouldn't break backwards migration for now, and only break it in future cases if it would really result in a broken device on the destination.

Quoted 4 lines · Kevin Wolf

Am 21.05.2015 um 12:11 hat Peter Maydell geschrieben:

Quoted 3 lines

2b add a subsection, send the subsection always in new qemu, if receiving from an old qemu then calculate the phase from the register fields in post-load

Doesn't this break new -> old migration?

Quoted 3 lines · Markus Armbruster

Quoted 1 line · Kevin Wolf

Am 21.05.2015 um 12:11 hat Peter Maydell geschrieben:

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

2b add a subsection, send the subsection always in new qemu, if receiving from an old qemu then calculate the phase from the register fields in post-load

Doesn't this break new -> old migration?

Do we care about that? Personally I would have thought it was too hard to defend and a very narrow use case compared to old->new.

-- PMM

Instead of relying on a flag in the MSR to distinguish controller phases, use the explicit phase that we store now. Assertions of the right MSR flags are added.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 67 ++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

hw/block/fdc.c +0 -0

index 4d4868e..a13e0ce 100644

Instead of relying on a flag in the MSR to distinguish controller phases, use the explicit phase that we store now. Assertions of the right MSR flags are added.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 67 ++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

hw/block/fdc.c +0 -0

index 4d4868e..a13e0ce 100644

Factor out a few common lines of code, reformat, improve comments.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 62 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 24 deletions(-)

hw/block/fdc.c +28 -9

index a13e0ce..cbf7abf 100644
@@ -1942,14 +1942,16 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
 /*
  * Handlers for the execution phase of each command
  */
-static const struct {
+typedef struct FDCtrlCommand {
     uint8_t value;
     uint8_t mask;
     const char* name;
     int parameters;
     void (*handler)(FDCtrl *fdctrl, int direction);
     int direction;
-} handlers[] = {
+} FDCtrlCommand;
+
+static const FDCtrlCommand handlers[] = {
     { FD_CMD_READ, 0x1f, "READ", 8, fdctrl_start_transfer, FD_DIR_READ },
     { FD_CMD_WRITE, 0x3f, "WRITE", 8, fdctrl_start_transfer, FD_DIR_WRITE },
     { FD_CMD_SEEK, 0xff, "SEEK", 2, fdctrl_handle_seek },

@@ -1986,9 +1988,19 @@ static const struct {
 /* Associate command to an index in the 'handlers' array */
 static uint8_t command_to_handler[256];
 
+static const FDCtrlCommand *get_command(uint8_t cmd)
+{
+    int idx;
+
+    idx = command_to_handler[cmd];
+    FLOPPY_DPRINTF("%s command\n", handlers[idx].name);
+    return &handlers[idx];
+}
+
 static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
 {
     FDrive *cur_drv;
+    const FDCtrlCommand *cmd;
     uint32_t pos;
 
     /* Reset mode */

@@ -2002,13 +2014,20 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
     }
     fdctrl->dsr &= ~FD_DSR_PWRDOWN;
 
+    FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
+
+    /* If data_len spans multiple sectors, the current position in the FIFO
+     * wraps around while fdctrl->data_pos is the real position in the whole
+     * request. */
+    pos = fdctrl->data_pos++;
+    pos %= FD_SECTOR_LEN;
+    fdctrl->fifo[pos] = value;
+
     switch (fdctrl->phase) {
     case FD_PHASE_EXECUTION:
         assert(fdctrl->msr & FD_MSR_NONDMA);
+
         /* FIFO data write */
-        pos = fdctrl->data_pos++;
-        pos %= FD_SECTOR_LEN;
-        fdctrl->fifo[pos] = value;
         if (pos == FD_SECTOR_LEN - 1 ||
             fdctrl->data_pos == fdctrl->data_len) {
             cur_drv = get_cur_drv(fdctrl);

@@ -2024,41 +2043,36 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
                 break;
             }
         }
-        /* Switch from transfer mode to status mode
-         * then from status mode to command mode
-         */
-        if (fdctrl->data_pos == fdctrl->data_len)
+
+        /* Switch to result phase when done with the transfer */
+        if (fdctrl->data_pos == fdctrl->data_len) {
             fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
+        }
         break;

hw/block/fdc.c +6 -7

+            /* The first byte specifies the command. Now we start reading
+             * as many parameters as this command requires. */
+            cmd = get_command(value);
+            fdctrl->data_len = cmd->parameters + 1;
             fdctrl->msr |= FD_MSR_CMDBUSY;
         }
 
-        FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
-        pos = fdctrl->data_pos++;
-        pos %= FD_SECTOR_LEN;
-        fdctrl->fifo[pos] = value;
         if (fdctrl->data_pos == fdctrl->data_len) {
-            /* We now have all parameters
-             * and will be able to treat the command
-             */
+            /* We have all parameters now, execute the command */
             fdctrl->phase = FD_PHASE_EXECUTION;
+
             if (fdctrl->data_state & FD_STATE_FORMAT) {
                 fdctrl_format_sector(fdctrl);
                 break;
             }

Quoted 92 lines

Factor out a few common lines of code, reformat, improve comments.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 62 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 24 deletions(-)

hw/block/fdc.c +28 -9

index a13e0ce..cbf7abf 100644
@@ -1942,14 +1942,16 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
 /*
  * Handlers for the execution phase of each command
  */
-static const struct {
+typedef struct FDCtrlCommand {
     uint8_t value;
     uint8_t mask;
     const char* name;
     int parameters;
     void (*handler)(FDCtrl *fdctrl, int direction);
     int direction;
-} handlers[] = {
+} FDCtrlCommand;
+
+static const FDCtrlCommand handlers[] = {
     { FD_CMD_READ, 0x1f, "READ", 8, fdctrl_start_transfer, FD_DIR_READ },
     { FD_CMD_WRITE, 0x3f, "WRITE", 8, fdctrl_start_transfer, FD_DIR_WRITE },
     { FD_CMD_SEEK, 0xff, "SEEK", 2, fdctrl_handle_seek },

@@ -1986,9 +1988,19 @@ static const struct {
 /* Associate command to an index in the 'handlers' array */
 static uint8_t command_to_handler[256];
 
+static const FDCtrlCommand *get_command(uint8_t cmd)
+{
+    int idx;
+
+    idx = command_to_handler[cmd];
+    FLOPPY_DPRINTF("%s command\n", handlers[idx].name);
+    return &handlers[idx];
+}
+
 static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
 {
     FDrive *cur_drv;
+    const FDCtrlCommand *cmd;
     uint32_t pos;
 
     /* Reset mode */

@@ -2002,13 +2014,20 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
     }
     fdctrl->dsr &= ~FD_DSR_PWRDOWN;
 
+    FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
+
+    /* If data_len spans multiple sectors, the current position in the FIFO
+     * wraps around while fdctrl->data_pos is the real position in the whole
+     * request. */
+    pos = fdctrl->data_pos++;
+    pos %= FD_SECTOR_LEN;
+    fdctrl->fifo[pos] = value;
+
     switch (fdctrl->phase) {
     case FD_PHASE_EXECUTION:
         assert(fdctrl->msr & FD_MSR_NONDMA);
+
         /* FIFO data write */
-        pos = fdctrl->data_pos++;
-        pos %= FD_SECTOR_LEN;
-        fdctrl->fifo[pos] = value;
         if (pos == FD_SECTOR_LEN - 1 ||
             fdctrl->data_pos == fdctrl->data_len) {
             cur_drv = get_cur_drv(fdctrl);

@@ -2024,41 +2043,36 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
                 break;
             }
         }
-        /* Switch from transfer mode to status mode
-         * then from status mode to command mode
-         */
-        if (fdctrl->data_pos == fdctrl->data_len)
+
+        /* Switch to result phase when done with the transfer */
+        if (fdctrl->data_pos == fdctrl->data_len) {
             fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
+        }
         break;
 
     case FD_PHASE_COMMAND:
         assert(!(fdctrl->msr & FD_MSR_NONDMA));
 

hw/block/fdc.c static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) · line 2056

Quoted 6 lines

hw/block/fdc.c +1 -5

-        if (fdctrl->data_pos == 0) {
-            /* Command */
-            pos = command_to_handler[value & 0xff];
-            FLOPPY_DPRINTF("%s command\n", handlers[pos].name);
-            fdctrl->data_len = handlers[pos].parameters + 1;
+        if (fdctrl->data_pos == 1) {

This reads more awkwardly than the previous ifz, but it's not like I have a better idea. (I just had a momentary pause of "Why 1?")

This commit makes similar improvements as have already been made to the write function: Instead of relying on a flag in the MSR to distinguish controller phases, use the explicit phase that we store now. Assertions of the right MSR flags are added.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

hw/block/fdc.c +8 -1

index cbf7abf..8d322e0 100644
@@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         FLOPPY_DPRINTF("error: controller not ready for reading\n");
         return 0;
     }
+
+    /* If data_len spans multiple sectors, the current position in the FIFO
+     * wraps around while fdctrl->data_pos is the real position in the whole
+     * request. */
     pos = fdctrl->data_pos;
     pos %= FD_SECTOR_LEN;
-    if (fdctrl->msr & FD_MSR_NONDMA) {
+
+    switch (fdctrl->phase) {
+    case FD_PHASE_EXECUTION:
+        assert(fdctrl->msr & FD_MSR_NONDMA);
         if (pos == 0) {
             if (fdctrl->data_pos != 0)
                 if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {

hw/block/fdc.c +0 -4

-        /* Switch from transfer mode to status mode
-         * then from status mode to command mode
-         */
-        if (fdctrl->msr & FD_MSR_NONDMA) {

hw/block/fdc.c +1 -0

             fdctrl_to_command_phase(fdctrl);
             fdctrl_reset_irq(fdctrl);
         }
+        break;

Quoted 35 lines

This commit makes similar improvements as have already been made to the write function: Instead of relying on a flag in the MSR to distinguish controller phases, use the explicit phase that we store now. Assertions of the right MSR flags are added.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

hw/block/fdc.c +8 -1

index cbf7abf..8d322e0 100644
@@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         FLOPPY_DPRINTF("error: controller not ready for reading\n");
         return 0;
     }
+
+    /* If data_len spans multiple sectors, the current position in the FIFO
+     * wraps around while fdctrl->data_pos is the real position in the whole
+     * request. */
     pos = fdctrl->data_pos;
     pos %= FD_SECTOR_LEN;
-    if (fdctrl->msr & FD_MSR_NONDMA) {
+
+    switch (fdctrl->phase) {
+    case FD_PHASE_EXECUTION:
+        assert(fdctrl->msr & FD_MSR_NONDMA);
         if (pos == 0) {
             if (fdctrl->data_pos != 0)
                 if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {

@@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
                 memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
             }

hw/block/fdc.c static uint32_t fdctrl_read_data(FDCtrl *fdctrl) · line 1557

Quoted 5 lines

hw/block/fdc.c +0 -4

         }
-    }
-    retval = fdctrl->fifo[pos];
-    if (++fdctrl->data_pos == fdctrl->data_len) {
-        fdctrl->data_pos = 0;

I suppose data_pos is now reset by either stop_transfer (via to_result_phase) or to_command_phase, so this is OK.

Quoted 10 lines

hw/block/fdc.c +4 -5

-        /* Switch from transfer mode to status mode
-         * then from status mode to command mode
-         */
-        if (fdctrl->msr & FD_MSR_NONDMA) {
+
+        if (++fdctrl->data_pos == fdctrl->data_len) {
             fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
-        } else {
+        }
+        break;

hw/block/fdc.c static uint32_t fdctrl_read_data(FDCtrl *fdctrl) · line 1569

Quoted 4 lines

hw/block/fdc.c +4 -0

+
+    case FD_PHASE_RESULT:
+        assert(!(fdctrl->msr & FD_MSR_NONDMA));
+        if (++fdctrl->data_pos == fdctrl->data_len) {

Not a terribly big fan of moving this pointer independently inside of each case statement, but I guess the alternative does look a lot worse. Having things separated by phases is a lot easier to follow.

Am 19.05.2015 um 22:40 hat John Snow geschrieben:

Quoted 38 lines · Kevin Wolf

Quoted 35 lines

This commit makes similar improvements as have already been made to the write function: Instead of relying on a flag in the MSR to distinguish controller phases, use the explicit phase that we store now. Assertions of the right MSR flags are added.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

hw/block/fdc.c +8 -1

index cbf7abf..8d322e0 100644
@@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         FLOPPY_DPRINTF("error: controller not ready for reading\n");
         return 0;
     }
+
+    /* If data_len spans multiple sectors, the current position in the FIFO
+     * wraps around while fdctrl->data_pos is the real position in the whole
+     * request. */
     pos = fdctrl->data_pos;
     pos %= FD_SECTOR_LEN;
-    if (fdctrl->msr & FD_MSR_NONDMA) {
+
+    switch (fdctrl->phase) {
+    case FD_PHASE_EXECUTION:
+        assert(fdctrl->msr & FD_MSR_NONDMA);
         if (pos == 0) {
             if (fdctrl->data_pos != 0)
                 if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {

@@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
                 memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
             }

Quoted 8 lines

hw/block/fdc.c static uint32_t fdctrl_read_data(FDCtrl *fdctrl) · line 1557

Quoted 5 lines

hw/block/fdc.c +0 -4

         }
-    }
-    retval = fdctrl->fifo[pos];
-    if (++fdctrl->data_pos == fdctrl->data_len) {
-        fdctrl->data_pos = 0;

I suppose data_pos is now reset by either stop_transfer (via to_result_phase) or to_command_phase, so this is OK.

Yes, that was redundant code.

Quoted 10 lines

Quoted 10 lines

hw/block/fdc.c +4 -5

-        /* Switch from transfer mode to status mode
-         * then from status mode to command mode
-         */
-        if (fdctrl->msr & FD_MSR_NONDMA) {
+
+        if (++fdctrl->data_pos == fdctrl->data_len) {
             fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
-        } else {
+        }
+        break;

Quoted 8 lines

hw/block/fdc.c static uint32_t fdctrl_read_data(FDCtrl *fdctrl) · line 1569

Quoted 4 lines

hw/block/fdc.c +4 -0

+
+    case FD_PHASE_RESULT:
+        assert(!(fdctrl->msr & FD_MSR_NONDMA));
+        if (++fdctrl->data_pos == fdctrl->data_len) {

Not a terribly big fan of moving this pointer independently inside of each case statement, but I guess the alternative does look a lot worse. Having things separated by phases is a lot easier to follow.

I'm not too happy about it either, but I couldn't think of anything better. Having two different switches almost immediately after each other, with only the if line in between, would look really awkward and be hard to read. And the old code isn't nice either.

If you have any idea for a better solution, let me know.

Kevin

The RQM bit in MSR should be set whenever the guest is supposed to access the FIFO, and it should be cleared in all other cases. This is important so the guest can't continue writing/reading the FIFO beyond the length that it's suppossed to access (see CVE-2015-3456).

Commit e9077462 fixed the CVE by adding code that avoids the buffer overflow; however it doesn't correct the wrong behaviour of the floppy controller which should already have cleared RQM.

Currently, RQM stays set all the time and during all phases while a command is being processed. This is error-prone because the command has to explicitly clear the flag if it doesn't need data (and indeed, the two buggy commands that are the culprits for the CVE just forgot to do that).

This patch clears RQM immediately as soon as all bytes that are expected have been received. If the the FIFO is used in the next phase, the flag has to be set explicitly there.

This alone should have been enough to fix the CVE, but now we have two lines of defense - even better.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

hw/block/fdc.c +3 -1

index 8d322e0..c6a046e 100644
@@ -1165,7 +1165,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
     fdctrl->phase = FD_PHASE_COMMAND;
     fdctrl->data_dir = FD_DIR_WRITE;
     fdctrl->data_pos = 0;
+    fdctrl->data_len = 1; /* Accept command byte, adjust for params later */
     fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
+    fdctrl->msr |= FD_MSR_RQM;
 }
 
 /* Update the state to allow the guest to read out the command status.

@@ -1380,7 +1382,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
         }
     }
     FLOPPY_DPRINTF("start non-DMA transfer\n");
-    fdctrl->msr |= FD_MSR_NONDMA;
+    fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM;
     if (direction != FD_DIR_WRITE)
         fdctrl->msr |= FD_MSR_DIO;
     /* IO based transfer: calculate len */

hw/block/fdc.c +7 -0

             fdctrl_to_command_phase(fdctrl);
             fdctrl_reset_irq(fdctrl);
         }

@@ -2036,6 +2040,10 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
     pos %= FD_SECTOR_LEN;
     fdctrl->fifo[pos] = value;
 
+    if (fdctrl->data_pos == fdctrl->data_len) {
+        fdctrl->msr &= ~FD_MSR_RQM;
+    }
+
     switch (fdctrl->phase) {
     case FD_PHASE_EXECUTION:
         assert(fdctrl->msr & FD_MSR_NONDMA);

@@ -2071,6 +2079,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
              * as many parameters as this command requires. */
             cmd = get_command(value);
             fdctrl->data_len = cmd->parameters + 1;
+            if (cmd->parameters) {
+                fdctrl->msr |= FD_MSR_RQM;
+            }
             fdctrl->msr |= FD_MSR_CMDBUSY;
         }
 

Quoted 53 lines

The RQM bit in MSR should be set whenever the guest is supposed to access the FIFO, and it should be cleared in all other cases. This is important so the guest can't continue writing/reading the FIFO beyond the length that it's suppossed to access (see CVE-2015-3456).

Commit e9077462 fixed the CVE by adding code that avoids the buffer overflow; however it doesn't correct the wrong behaviour of the floppy controller which should already have cleared RQM.

Currently, RQM stays set all the time and during all phases while a command is being processed. This is error-prone because the command has to explicitly clear the flag if it doesn't need data (and indeed, the two buggy commands that are the culprits for the CVE just forgot to do that).

This patch clears RQM immediately as soon as all bytes that are expected have been received. If the the FIFO is used in the next phase, the flag has to be set explicitly there.

This alone should have been enough to fix the CVE, but now we have two lines of defense - even better.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

hw/block/fdc.c +3 -1

index 8d322e0..c6a046e 100644
@@ -1165,7 +1165,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
     fdctrl->phase = FD_PHASE_COMMAND;
     fdctrl->data_dir = FD_DIR_WRITE;
     fdctrl->data_pos = 0;
+    fdctrl->data_len = 1; /* Accept command byte, adjust for params later */
     fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
+    fdctrl->msr |= FD_MSR_RQM;
 }
 
 /* Update the state to allow the guest to read out the command status.

@@ -1380,7 +1382,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
         }
     }
     FLOPPY_DPRINTF("start non-DMA transfer\n");
-    fdctrl->msr |= FD_MSR_NONDMA;
+    fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM;
     if (direction != FD_DIR_WRITE)
         fdctrl->msr |= FD_MSR_DIO;
     /* IO based transfer: calculate len */

@@ -1560,6 +1562,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         }
 

hw/block/fdc.c static uint32_t fdctrl_read_data(FDCtrl *fdctrl) · line 1565

Quoted 2 lines

hw/block/fdc.c +1 -0

         if (++fdctrl->data_pos == fdctrl->data_len) {
+            fdctrl->msr &= ~FD_MSR_RQM;

Doesn't stop_transfer set this flag back right away?

Am 19.05.2015 um 22:40 hat John Snow geschrieben:

Quoted 56 lines · Kevin Wolf

Quoted 53 lines

The RQM bit in MSR should be set whenever the guest is supposed to access the FIFO, and it should be cleared in all other cases. This is important so the guest can't continue writing/reading the FIFO beyond the length that it's suppossed to access (see CVE-2015-3456).

Commit e9077462 fixed the CVE by adding code that avoids the buffer overflow; however it doesn't correct the wrong behaviour of the floppy controller which should already have cleared RQM.

Currently, RQM stays set all the time and during all phases while a command is being processed. This is error-prone because the command has to explicitly clear the flag if it doesn't need data (and indeed, the two buggy commands that are the culprits for the CVE just forgot to do that).

This patch clears RQM immediately as soon as all bytes that are expected have been received. If the the FIFO is used in the next phase, the flag has to be set explicitly there.

This alone should have been enough to fix the CVE, but now we have two lines of defense - even better.

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/block/fdc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

hw/block/fdc.c +3 -1

index 8d322e0..c6a046e 100644
@@ -1165,7 +1165,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
     fdctrl->phase = FD_PHASE_COMMAND;
     fdctrl->data_dir = FD_DIR_WRITE;
     fdctrl->data_pos = 0;
+    fdctrl->data_len = 1; /* Accept command byte, adjust for params later */
     fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
+    fdctrl->msr |= FD_MSR_RQM;
 }
 
 /* Update the state to allow the guest to read out the command status.

@@ -1380,7 +1382,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
         }
     }
     FLOPPY_DPRINTF("start non-DMA transfer\n");
-    fdctrl->msr |= FD_MSR_NONDMA;
+    fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM;
     if (direction != FD_DIR_WRITE)
         fdctrl->msr |= FD_MSR_DIO;
     /* IO based transfer: calculate len */

@@ -1560,6 +1562,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         }
 

Quoted 4 lines

hw/block/fdc.c static uint32_t fdctrl_read_data(FDCtrl *fdctrl) · line 1565

Quoted 2 lines

hw/block/fdc.c +1 -0

         if (++fdctrl->data_pos == fdctrl->data_len) {
+            fdctrl->msr &= ~FD_MSR_RQM;

Doesn't stop_transfer set this flag back right away?

It does, by switching to the result phase.

I think it's clearer to disable the bit anywhere where the FIFO has received as many bytes as it's supposed to, even if the next phase is started immediately and reenables it.

In real hardware, sending a byte causes the FDC to disable RQM, then process the byte (which means completing command execution for this code path), then reenable RQM if needed.

Currently our code is completely synchronous, so we could ignore this detail because the state between clearing and setting RQM isn't observable by the guest. If we ever introduce something asynchronous in the path, we will need this though - and modelling real hardware more precisely has never hurt anyway.

Kevin

This just adds a few additional checks of the MSR and interrupt pin to the already existing test cases.

Signed-off-by Kevin Wolf <[email protected]>

---
 tests/fdc-test.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

tests/fdc-test.c +0 -0

index 3c6c83c..416394f 100644

This series fixes the real bug that caused CVE-2015-3456, and does some cleanup in the FIFO access functions to make the command processing more obvious.

Kevin Wolf (8):
fdc: Rename fdctrl_reset_fifo() to fdctrl_to_command_phase()
fdc: Rename fdctrl_set_fifo() to fdctrl_to_result_phase()
fdc: Introduce fdctrl->phase
fdc: Use phase in fdctrl_write_data()
fdc: Code cleanup in fdctrl_write_data()
fdc: Disentangle phases in fdctrl_read_data()
fdc: Fix MSR.RQM flag
fdc-test: Test state for existing cases more thoroughly

 hw/block/fdc.c   | 235 +++++++++++++++++++++++++++++++++++++------------------
 tests/fdc-test.c |  34 ++++++++
 2 files changed, 192 insertions(+), 77 deletions(-)