linux-kernel - Re: [PATCH v2 1/3] Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus hardening

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200630104856.i3zwib2zrf644w6v@liuwe-devbox-debian-v2>
Date:   Tue, 30 Jun 2020 10:48:56 +0000
From:   Wei Liu <wei.liu@...nel.org>
To:     Andres Beltran <lkmlabelt@...il.com>
Cc:     Wei Liu <wei.liu@...nel.org>,
        Andres Beltran <t-mabelt@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
        Michael Kelley <mikelley@...rosoft.com>,
        Andrea Parri <parri.andrea@...il.com>
Subject: Re: [PATCH v2 1/3] Drivers: hv: vmbus: Add vmbus_requestor data
 structure for VMBus hardening

On Tue, Jun 30, 2020 at 10:17:36AM +0000, Wei Liu wrote:
[...]
> > > 
> > > If the allocation of the requestor fails during runtime, vmbus_open()
> > > fails too and therefore,
> > > the channel and the requestor will not be created. So, the 2 functions
> > > (next_id, requestor_addr)
> > > will never get called, right? The only case in which we hit this edge
> > > case is if a driver is using this
> > > mechanism with a size of 0 (i.e. rqstor_size is not set to a non-zero
> > > value before calling vmbus_open()),
> > 
> > Right. This is what I was getting at. Setting the size to 0 effectively
> > makes the driver unusable. And per your design, it should be considered
> > a bug.
> > 
> > > but that would be more like a coding bug. So, I think it would be
> > > better to return VMBUS_RQST_ERROR
> > > as a way to assert that there is a bug in the code. I don't know if
> > > I'm missing something here.
> > 
> > Since we know setting size to 0 is a bug, you can actually just do the
> > following in the __vmbus_open function instead of going through all the
> > initialization with the knowledge vmbus_next_request_id & co will fail.
> > 
> >     /* Create and init requestor */
> >     if (!newchannel->rqstor_size)
> >           return an error to caller here
> > 
> >     vmbus_alloc_requestor(...);
> 
> And obviously you should check vmbus_alloc_requestor's return value
> somehow. You get the idea...
> 

Andrea pointed out that I missed one critical aspect of the design --
not all drivers are supposed to use this infrastructure. That's contrary
to my original understanding, in which all drivers are supposed to use
this infrastructure.

With that in mind, it is okay to only initialize the infra only when
->rqstor_size is not zero. Then you just handle the edge case in
vmbus_next_request_id & co.

Wei.

> Wei.
> 
> > 
> > 
> > Wei.
> > 
> > > 
> > > Andres.