No subject

[bogus at does.not.exist.com]

update the EAP peer config was overkill. At the time, I thought that I
found where the eapol sm was being used as a parameter for a callback
on the eloop and that during the time from when it was registered to
when it was called back, NM would come along and send a
SetSmartcardModules DBus message which would invalidate it and the
segfault was just a matter of time after that. Unfortunately, I don't
have those notes anymore and I can't find a specific reference.

So this fix was to make the updating of the EAP config more granular
which is a good thing, even if this is not the best way. As you can
see from the patch, there are already two places in the code that need
an interface to do this (though they are both from updates to
smartcard modules and that maybe they only relevant case) so I didn't
want to create a bigger solution just for
wpas_dbus_iface_set_smartcard_modules(). The patch completely fixes my
segfaulting.

>
> The EAPOL code should already be notified of configuration changes with
> eapol_sm_notify_config() call. Since EAPOL is the owner of EAP, this EAP
> peer update should probably be done from the eapol_sm_notify_config()
> call instead of providing a separate new function for this
> functionality to bypass most of EAPOL processing.

I thought about this a lot before eventually deciding on adding a new
function. In the end, it seemed that since updating the global EAP
config will have to create an entire new EAP peer state machine, it's
best done through a separate interface.

> It would also be closer to EAPOL design if a new EAP peer function were
> added to notify it about configuration changes instead of doing this
> with full deinit/init.

Yeah that would be better :) So instead of the function I've added in
this patch, instead lets add a function to src/eap_peer/eap.c like
this (pseudo code):

void eap_sm_update_config(struct eap_sm *sm, struct eap_config *conf)
{
   eap_invalidate_cached_session(sm)
   eap_sm_abort(sm)
   if conf->mac_addr, copy to sm->mac_addr
   tls_deinit(sm->ssl_ctx)
   create a tlsconf object based on the values in conf
   sm->ssl_ctx =3D tls_init(&tlsconf), maybe die on failure
}

>
> I do not like the part of getting from one to three places where the
> struct eap_config is filled in. It is prone to errors (this patch being
> a good example of this by breaking WPS ;-). It would be better to get
> that part into a shared function so that only one place needs to be
> modified whenever adding new entries into struct eap_config.

OK, so another function in src/eap_peer/eap.c for updating smartcard
modules only and it would internally create the eap_config object and
pass that to the above eap_sm_update_config?

> Getting both DBus and configuration file reload handlers doing the same
> thing is good direction, but unless I have missed something, I would
> prefer this to go through eapol_sm_notify_config() with likely a new
> helper function in wpas_glue.c that can be used to get the updated
> smartcard parameters first into EAPOL and then from the EAPOL code into
> EAP.

So, first, is it then OK to make eapol_sm_notify_config stop the
current running EAP state machine? Because that's necessary to update
the ssl_ctx. Second, how would you want to pass the struct eap_config
reference around?

Cheers,

>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP at lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap



--
- dds