Re: [PATCH v2 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic32

[Shuah Khan]

On 10/7/20 12:27 PM, Kees Cook wrote:
> On Tue, Oct 06, 2020 at 02:44:41PM -0600, Shuah Khan wrote:
> > counter_atomic* is introduced to be used when a variable is used as
> > a simple counter and doesn't guard object lifetimes. This clearly
> > differentiates atomic_t usages that guard object lifetimes.
> >
> > counter_atomic* variables will wrap around to 0 when it overflows and
> > should not be used to guard resource lifetimes, device usage and
> > open counts that control state changes, and pm states.
> >
> > atomic_t variable used to count number of vmci guest devices is used
> > as just as counter and it doesn't control object lifetimes or state
> > management. Overflow doesn't appear to be problem for this use.
> >
> > Convert it to use counter_atomic32.
> >
> > This conversion doesn't change the overflow wrap around behavior.
> >
> > Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
>
> I'm not convinced this isn't both managing lifetime and already buggy.
> Specifically, I'm looking at how vmci_guest_code_active() is used --
> it's being tested before making calls? Is this safe?

It is being used as a flag in the sense that !=0 indicates that
there is a guest instance. This counter value isn't used in any
code paths.

> > ---
> >   drivers/misc/vmw_vmci/vmci_guest.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
> > index cc8eeb361fcd..86ae27b05fc2 100644
> > --- a/drivers/misc/vmw_vmci/vmci_guest.c
> > +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/smp.h>
> >   #include <linux/io.h>
> >   #include <linux/vmalloc.h>
> > +#include <linux/counters.h>
> >   
> >   #include "vmci_datagram.h"
> >   #include "vmci_doorbell.h"
> > @@ -68,11 +69,11 @@ struct pci_dev *vmci_pdev;
> >   static struct vmci_guest_device *vmci_dev_g;
> >   static DEFINE_SPINLOCK(vmci_dev_spinlock);
> >   
> > -static atomic_t vmci_num_guest_devices = ATOMIC_INIT(0);
> > +static struct counter_atomic32 vmci_num_guest_devices = COUNTER_ATOMIC_INIT(0);
> >   
> >   bool vmci_guest_code_active(void)
> >   {
> > -	return atomic_read(&vmci_num_guest_devices) != 0;
> > +	return counter_atomic32_read(&vmci_num_guest_devices) != 0;
>
> Shouldn't this be "> 0" ?

Correct. > 0 would be the right check here in the context this
counter is being used, however, I don't think this value will
ever get close overflow. Please see below.

> >   }
> >   
> >   u32 vmci_get_vm_context_id(void)
> > @@ -624,7 +625,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> >   
> >   	dev_dbg(&pdev->dev, "Registered device\n");
> >   
> > -	atomic_inc(&vmci_num_guest_devices);
> > +	counter_atomic32_inc(&vmci_num_guest_devices);
> >   
> >   	/* Enable specific interrupt bits. */
> >   	cmd = VMCI_IMR_DATAGRAM;
> > @@ -684,7 +685,7 @@ static void vmci_guest_remove_device(struct pci_dev *pdev)
> >   
> >   	dev_dbg(&pdev->dev, "Removing device\n");
> >   
> > -	atomic_dec(&vmci_num_guest_devices);
> > +	counter_atomic32_dec(&vmci_num_guest_devices);
>
> If there is a bug elsewhere and vmci_guest_remove_device() (or probe)
> gets called too many times, shouldn't we protect the rest of this stack
> from having vmci_num_guest_devices go negative (and therefore non-zero)?

vmci_num_guest_devices overflow/underflow causing stack corruption is
a minor problems compared to what could happen if _probe gets called
many times, considering the number of pci managed devices we would end
up with. In the sequence of things, we would run out of devm resources
to begin with.

vmci_num_guest_devices is used as bool flag really to determine if a
guest instance exists.

This driver has lots of problems that need to be addressed.

> This really seems like it should be refcount_t to me, though I have no
> idea what the races between the dec() and the read() might mean in this
> code generally.

I don't believe so. vmci_num_guest_devices is used as a status as one
of the factors to make decisions. The way it is being used atomic32 is
just fine.

This patch doesn't solve these problems and add any new ones.

thanks,
-- Shuah