linux-kernel - Re: [PATCH v3] USB:gadget: Fix a warning while loading g_mass

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 for Android: free password hash cracker in your pocket

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

Message-ID: <xa1t8up0oo0b.fsf@mina86.com>
Date:	Fri, 13 Jun 2014 16:57:56 +0200
From:	Michal Nazarewicz <mina86@...a86.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Wei.Yang@...driver.com, balbi@...com, andrzej.p@...sung.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] USB:gadget: Fix a warning while loading g_mass_storage

On Fri, Jun 13 2014, Alan Stern <stern@...land.harvard.edu> wrote:
> On Fri, 13 Jun 2014, Michal Nazarewicz wrote:
>
>> > The root cause is that the existing code fails to take into
>> > account the possibility that common->new_fsg can change while
>> > do_set_interface() is running, because the spinlock isn't held
>> > at this point.
>> 
>> common->new_fsg is not protected by common->lock so this justification
>> is not valid.
>
> That's true.  A better justification would be that the same value of 
> new_fsg should be used in do_set_interface() and in the test that 
> follows.
>
>> > @@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common)
>> >  		}
>> >  		common->state = FSG_STATE_IDLE;
>> >  	}
>> > +	new_fsg = common->new_fsg;
>> 
>> Also, because common->new_fsg is not protected by common->lock, doing
>> this under a lock is kinda pointless.
>
> Yes, but it doesn't hurt.
>
>> >  	spin_unlock_irq(&common->lock);
>> >  
>> >  	/* Carry out any extra actions required for the exception */
>> > @@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common)
>> >  		break;
>> >  
>> >  	case FSG_STATE_CONFIG_CHANGE:
>> > -		do_set_interface(common, common->new_fsg);
>> > -		if (common->new_fsg)
>> > +		do_set_interface(common, new_fsg);
>> > +		if (new_fsg)
>> >  			usb_composite_setup_continue(common->cdev);
>> 
>> As far as I can tell, it's safe to move the assignment to new_fsg here,
>> e.g.:
>> 
>> 		new_fsg = common->new_fsg;
>> 		do_set_interface(common, new_fsg);
>> 		if (new_fsg)
>> 			usb_composite_setup_continue(common->cdev);
>
> That would be equally correct.  I don't see any strong reason for 
> preferring one over the other.

Doing the read under the lock may give an impression that the value is
indeed protected by the lock.  Doing it outside of the lock creates no
such confusion.  Therefore, I would prefer having the read outside.  But
no strong feelings.

>> >  		break;
>> 
>> But perhaps new_fsg should be protected by the lock.  I think valid fix
>> (which I did not test in *any* way) will be this:

> No, I think this change is both too big and unnecessary.  
> common->new_fsg does not need protection; in a sense it is "owned" by 
> the composite driver.

No strong feelings on my side.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@...gle.com>--<xmpp:mina86@...ber.org>--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/