[Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Ian Jackson]

I've picked up Alex's patch series for this.  Changes since v4:
  * rebased to current tip as of approximately yesterday
  * passthrough of firmware update is enabled by default
  * fixed some build issues

Anthony Liguori wrote:
> This series breaks the cris-softmmu build.

I think I've fixed it up.  It builds for me, anyway.  If it now
doesn't build do please tell me the error message and I'l fix it.

> Also, I think Paul and I both requested that fw upgrade not be
> disabled by default.

As previously discussed I think this is a mistake, but it's a decision
for qemu upstream to make so I have changed this.

>   I would also suggest that you only expose this as an option 
> through qdev properties instead of a new command line option as it 
> should be controllable on a per-device basis.

The reason to disable it is not to prevent the guest breaking the
hardware.  It is to prevent the guest escaping the containment
entirely, which it can probably do if firmware updates are allowed.
This seems to me to be a general property of the guest, rather than of
the device.  So I think disabling it in one place is better.

Patches follow.

Thanks,
Ian.

[Qemu-devel] Re: [PATCH 0/7] ATAPI CDROM passthrough v5

[Ian Jackson]

I wrote:
> I think I've fixed it up.  It builds for me, anyway.  If it now
> doesn't build do please tell me the error message and I'l fix it.

I did have to do this, to fix an unrelated `may be used initialised'
warning.  I'm not sure if it's the right fix.

Ian.

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 4da916c..2efd21c 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -99,7 +99,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
                                            const char *opts)
 {
     PCIDevice *dev;
-    DriveInfo *dinfo;
+    DriveInfo *dinfo=0;
     int type = -1;
     char buf[128];
 

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Christoph Hellwig]

On Wed, Aug 12, 2009 at 06:18:13PM +0100, Ian Jackson wrote:
> I've picked up Alex's patch series for this.  Changes since v4:
>   * rebased to current tip as of approximately yesterday
>   * passthrough of firmware update is enabled by default
>   * fixed some build issues

If you resend patch from someone else the normal convention is to put
a

	From: Original Author <author@foobar.blah>

into the patch body so that it's obvious who the author is.

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Ian Jackson]

Christoph Hellwig writes ("Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5"):
> If you resend patch from someone else the normal convention is to put
> a
> 	From: Original Author <author@foobar.blah>
> into the patch body so that it's obvious who the author is.

OK, I will do that next time.

(I thought the Signed-Off-By would be sufficient but I'm happy to do
whatever people think best.)

Ian.

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Anthony Liguori]

Ian Jackson wrote:
>>   I would also suggest that you only expose this as an option 
>> through qdev properties instead of a new command line option as it 
>> should be controllable on a per-device basis.
>>     
>
> The reason to disable it is not to prevent the guest breaking the
> hardware.  It is to prevent the guest escaping the containment
> entirely, which it can probably do if firmware updates are allowed.
> This seems to me to be a general property of the guest, rather than of
> the device.  So I think disabling it in one place is better.
>   

If you go back to the original thread, the argument against this was 
that some devices abuse other atapi commands to do firmware updates so 
you cannot 100% reliably contain this.

But more importantly, and the reason I originally requested this, having 
a global option bakes knowledge of atapi pass through into vl.c.  Making 
it a qdev property means vl.c does not need explicit knowledge of this 
mechanism.

I think this is an important change to make for merging.

Regards,

Anthony Liguori

> Patches follow.
>
> Thanks,
> Ian.
>
>
>   

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Bique Alexandre]

On Wednesday 12 August 2009 17:18:13 Ian Jackson wrote:
> I've picked up Alex's patch series for this.  Changes since v4:
>   * rebased to current tip as of approximately yesterday
>   * passthrough of firmware update is enabled by default
>   * fixed some build issues
>
> Anthony Liguori wrote:
> > This series breaks the cris-softmmu build.
>
> I think I've fixed it up.  It builds for me, anyway.  If it now
> doesn't build do please tell me the error message and I'l fix it.
>
> > Also, I think Paul and I both requested that fw upgrade not be
> > disabled by default.
>
> As previously discussed I think this is a mistake, but it's a decision
> for qemu upstream to make so I have changed this.
>
> >   I would also suggest that you only expose this as an option
> > through qdev properties instead of a new command line option as it
> > should be controllable on a per-device basis.
>
> The reason to disable it is not to prevent the guest breaking the
> hardware.  It is to prevent the guest escaping the containment
> entirely, which it can probably do if firmware updates are allowed.
> This seems to me to be a general property of the guest, rather than of
> the device.  So I think disabling it in one place is better.
>
> Patches follow.

Hi Ian,

Are you going to continue to work on this patch queue or do you want me to 
finish the job ?

Thanks a lot.

-- 
Alexandre Bique

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Carl-Daniel Hailfinger]

On 28.08.2009 22:21, Bique Alexandre wrote:
> On Wednesday 12 August 2009 17:18:13 Ian Jackson wrote:
>   
>>> Also, I think Paul and I both requested that fw upgrade not be
>>> disabled by default.
>>>       
>> As previously discussed I think this is a mistake, but it's a decision
>> for qemu upstream to make so I have changed this.
>>     

Anyone up for writing a security advisory about this?

Regards,
Carl-Daniel

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Anthony Liguori]

Carl-Daniel Hailfinger wrote:
> On 28.08.2009 22:21, Bique Alexandre wrote:
>   
>> On Wednesday 12 August 2009 17:18:13 Ian Jackson wrote:
>>   
>>     
>>>> Also, I think Paul and I both requested that fw upgrade not be
>>>> disabled by default.
>>>>       
>>>>         
>>> As previously discussed I think this is a mistake, but it's a decision
>>> for qemu upstream to make so I have changed this.
>>>     
>>>       
>
> Anyone up for writing a security advisory about this?\
>   

Eh?

If you do hardware passthrough, the guest can mess up the device.  This 
is always going to be true and it's a security problem IMHO to make the 
user think anything other than that.

Regards,

Anthony Liguori

> Regards,
> Carl-Daniel
>
>
>   

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Carl-Daniel Hailfinger]

On 29.08.2009 22:49, Anthony Liguori wrote:
> Carl-Daniel Hailfinger wrote:
>> On 28.08.2009 22:21, Bique Alexandre wrote:
>>  
>>> On Wednesday 12 August 2009 17:18:13 Ian Jackson wrote:
>>>      
>>>>> Also, I think Paul and I both requested that fw upgrade not be
>>>>> disabled by default.
>>>>>               
>>>> As previously discussed I think this is a mistake, but it's a decision
>>>> for qemu upstream to make so I have changed this. 
>>
>> Anyone up for writing a security advisory about this?
>
> Eh?
>
> If you do hardware passthrough, the guest can mess up the device. 
> This is always going to be true and it's a security problem IMHO to
> make the user think anything other than that.

The guest can also mess up other devices with the help of specially
crafted firmware. So even if the user does not care about the effects on
a particular device, a firmware upgrade might affect other devices
(which are not used by Qemu in any way) as well. As a result, this is
essentially a "break out of qemu or DoS the machine under certain
conditions" feature. If that particular side effect / feature is
documented, users who read the documentation won't get any nasty surprises.
If that's what you intended to say, I apologize for the misunderstanding.

Regards,
Carl-Daniel

> Regards,
>
> Anthony Liguori

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Anthony Liguori]

Carl-Daniel Hailfinger wrote:
> The guest can also mess up other devices with the help of specially
> crafted firmware. So even if the user does not care about the effects on
> a particular device, a firmware upgrade might affect other devices
> (which are not used by Qemu in any way) as well.

Please be more specific.  How is this any different than PCI passthrough 
with VT-d or USB passthrough?

>  As a result, this is
> essentially a "break out of qemu or DoS the machine under certain
> conditions" feature. If that particular side effect / feature is
> documented, users who read the documentation won't get any nasty surprises.
>   

A user will get a really nasty surprise if they think they can use a 
flag or rely on QEMU to prevent a VM from doing something nasty with a 
device.  If they have this feeling of security, they're likely to chmod 
the device to allow unprivileged users to access it.

But how a device handles ATAPI commands is totally up to the device.  If 
you issue the wrong sequence, I'm sure there are devices out there that 
totally hose themselves.  Are you absolutely confident that every ATAPI 
device out there is completely safe against hostile code provided that 
you simply prevent the FW update commands?  I'm certainly not.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Alexander Graf]

On 30.08.2009, at 02:14, Anthony Liguori wrote:

> Carl-Daniel Hailfinger wrote:
>> The guest can also mess up other devices with the help of specially
>> crafted firmware. So even if the user does not care about the effects on
>> a particular device, a firmware upgrade might affect other devices
>> (which are not used by Qemu in any way) as well.
> 
> Please be more specific.  How is this any different than PCI passthrough with VT-d or USB passthrough?
> 
>> As a result, this is
>> essentially a "break out of qemu or DoS the machine under certain
>> conditions" feature. If that particular side effect / feature is
>> documented, users who read the documentation won't get any nasty surprises.
>>  
> 
> A user will get a really nasty surprise if they think they can use a flag or rely on QEMU to prevent a VM from doing something nasty with a device.  If they have this feeling of security, they're likely to chmod the device to allow unprivileged users to access it.
> 
> But how a device handles ATAPI commands is totally up to the device.  If you issue the wrong sequence, I'm sure there are devices out there that totally hose themselves.  Are you absolutely confident that every ATAPI device out there is completely safe against hostile code provided that you simply prevent the FW update commands?  I'm certainly not.

Ping?


Alex

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Anthony Liguori]

On 10/18/2010 06:29 PM, Alexander Graf wrote:
>> A user will get a really nasty surprise if they think they can use a flag or rely on QEMU to prevent a VM from doing something nasty with a device.  If they have this feeling of security, they're likely to chmod the device to allow unprivileged users to access it.
>>
>> But how a device handles ATAPI commands is totally up to the device.  If you issue the wrong sequence, I'm sure there are devices out there that totally hose themselves.  Are you absolutely confident that every ATAPI device out there is completely safe against hostile code provided that you simply prevent the FW update commands?  I'm certainly not.
>>      
> Ping?
>    

Who are you pinging?

Regards,

Anthony Liguori

> Alex
>
>
>    

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Alexander Graf]

Am 19.10.2010 um 02:10 schrieb Anthony Liguori <anthony@codemonkey.ws>:

> On 10/18/2010 06:29 PM, Alexander Graf wrote:
>>> A user will get a really nasty surprise if they think they can use a flag or rely on QEMU to prevent a VM from doing something nasty with a device.  If they have this feeling of security, they're likely to chmod the device to allow unprivileged users to access it.
>>> 
>>> But how a device handles ATAPI commands is totally up to the device.  If you issue the wrong sequence, I'm sure there are devices out there that totally hose themselves.  Are you absolutely confident that every ATAPI device out there is completely safe against hostile code provided that you simply prevent the FW update commands?  I'm certainly not.
>>>     
>> Ping?
>>   
> 
> Who are you pinging?

Mostly Ian. I haven't seen any follow-up on this discussion and would like to know why and if there's still plans to upstream this code :).

Alex

> 

Re: [Qemu-devel] [PATCH 0/7] ATAPI CDROM passthrough v5

[Michal Suchanek]

On 19 October 2010 08:17, Alexander Graf <agraf@suse.de> wrote:
>
> Am 19.10.2010 um 02:10 schrieb Anthony Liguori <anthony@codemonkey.ws>:
>
>> On 10/18/2010 06:29 PM, Alexander Graf wrote:
>>>> A user will get a really nasty surprise if they think they can use a flag or rely on QEMU to prevent a VM from doing something nasty with a device.  If they have this feeling of security, they're likely to chmod the device to allow unprivileged users to access it.
>>>>
>>>> But how a device handles ATAPI commands is totally up to the device.  If you issue the wrong sequence, I'm sure there are devices out there that totally hose themselves.  Are you absolutely confident that every ATAPI device out there is completely safe against hostile code provided that you simply prevent the FW update commands?  I'm certainly not.
>>>>
>>> Ping?
>>>
>>
>> Who are you pinging?
>
> Mostly Ian. I haven't seen any follow-up on this discussion and would like to know why and if there's still plans to upstream this code :).
>

Why is allowing ATAPI passthrough such a problem?

Sure if your boot drive is on the same IDE cable as the device you may
have issues but other than that the device may just stop working if it
is not designed to handle incorrect command gracefully (ie it is
broken).

I am sure there are devices that also break under issuing correct
commands or commands that look vaguely sane. Eg. there are CD-ROMs
that would lock up the whole system when you boot certain vintage of
Linux (not tested with current Linux due to lack of old hardware) on a
machine with the Intel BX chipset and one of these CD-ROMs attached
over IDE cable.

However, assuming random hardware breakage you cannot allow anything.

Perhaps the ATAPI passthrough should be designed to allow any commands
and some command profiles could be selected to allow for some
sane/conservative subset, burning, LightScribe, LabelFlash, disc
T@tto, FW upgrade, ..

It would be nice if these subsets were defined in a configuration file
so that people can create their own 'default' combination or just
install a new set when a new fancy feature comes out.

Thanks

Michal