Message ID | b1f4175e7c83e04f9484ada7fc94ba4df92495eb.1474298427.git.sd@queasysnail.net |
---|---|
State | Changes Requested |
Headers | show |
On Tue, Sep 20, 2016 at 09:43:11AM +0200, Sabrina Dubroca wrote: > src/drivers/driver.h | 2 ++ > src/pae/ieee802_1x_kay.c | 15 +++++++++++++-- > src/pae/ieee802_1x_kay.h | 1 + > src/pae/ieee802_1x_secy_ops.c | 20 ++++++++++++++++++++ > src/pae/ieee802_1x_secy_ops.h | 1 + > wpa_supplicant/driver_i.h | 9 +++++++++ > wpa_supplicant/wpas_kay.c | 7 +++++++ > 7 files changed, 53 insertions(+), 2 deletions(-) So no changes to src/drivers/driver_macsec_qca.c? > * enable_protect_frames - Set protect frames status > * @priv: Private driver interface data > diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c > @@ -3412,6 +3417,12 @@ ieee802_1x_kay_change_cipher_suite(struct ieee802_1x_kay *kay, > kay->macsec_csindex = cs_index; > kay->macsec_capable = cipher_suite_tbl[kay->macsec_csindex].capable; > > + if (secy_get_capability(kay, &secy_cap) < 0) > + return -3; Wouldn't this call to secy_get_capability() return -1 for unmodified driver_macsec_qca.c and as such, this patch would break that driver wrapper?
2016-10-03, 13:32:10 +0300, Jouni Malinen wrote: > On Tue, Sep 20, 2016 at 09:43:11AM +0200, Sabrina Dubroca wrote: > > > src/drivers/driver.h | 2 ++ > > src/pae/ieee802_1x_kay.c | 15 +++++++++++++-- > > src/pae/ieee802_1x_kay.h | 1 + > > src/pae/ieee802_1x_secy_ops.c | 20 ++++++++++++++++++++ > > src/pae/ieee802_1x_secy_ops.h | 1 + > > wpa_supplicant/driver_i.h | 9 +++++++++ > > wpa_supplicant/wpas_kay.c | 7 +++++++ > > 7 files changed, 53 insertions(+), 2 deletions(-) > > So no changes to src/drivers/driver_macsec_qca.c? > > > * enable_protect_frames - Set protect frames status > > * @priv: Private driver interface data > > diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c > > @@ -3412,6 +3417,12 @@ ieee802_1x_kay_change_cipher_suite(struct ieee802_1x_kay *kay, > > kay->macsec_csindex = cs_index; > > kay->macsec_capable = cipher_suite_tbl[kay->macsec_csindex].capable; > > > > + if (secy_get_capability(kay, &secy_cap) < 0) > > + return -3; > > Wouldn't this call to secy_get_capability() return -1 for unmodified > driver_macsec_qca.c and as such, this patch would break that driver > wrapper? Not really, because (luckily?) this function (ieee802_1x_kay_change_cipher_suite) is never called. In ieee802_1x_kay_init I added a fallback so that if a driver doesn't tell us its capability, we assume it can do everything. But, yes, this is broken. I see a few options here: 1) fallback in both ieee802_1x_kay_init and ieee802_1x_kay_change_cipher_suite. 2) fallback, and implement macsec_get_capability op in driver_macsec_qca.c. 3) no fallback, just implement macsec_get_capability op in driver_macsec_qca.c. I'd lean towards option 3 and will update the patch, unless you prefer something else? Thanks,
On Wed, Oct 05, 2016 at 11:19:00AM +0200, Sabrina Dubroca wrote: > Not really, because (luckily?) this function > (ieee802_1x_kay_change_cipher_suite) is never called. Interesting.. Maybe we should just remove that function unless you had some plans of taking it into use in the future. > In > ieee802_1x_kay_init I added a fallback so that if a driver doesn't > tell us its capability, we assume it can do everything. But, yes, > this is broken. > > I see a few options here: > > 1) fallback in both ieee802_1x_kay_init and > ieee802_1x_kay_change_cipher_suite. > 2) fallback, and implement macsec_get_capability op in > driver_macsec_qca.c. > 3) no fallback, just implement macsec_get_capability op in > driver_macsec_qca.c. > > I'd lean towards option 3 and will update the patch, unless you prefer > something else? Option 3 sounds fine to me.
diff --git a/src/drivers/driver.h b/src/drivers/driver.h index 61877cae963a..8bd8fb87014a 100644 --- a/src/drivers/driver.h +++ b/src/drivers/driver.h @@ -3297,6 +3297,8 @@ struct wpa_driver_ops { int (*macsec_deinit)(void *priv); + int (*macsec_get_capability)(void *priv, enum macsec_cap *cap); + /** * enable_protect_frames - Set protect frames status * @priv: Private driver interface data diff --git a/src/pae/ieee802_1x_kay.c b/src/pae/ieee802_1x_kay.c index ce996bd80440..72ecaaa6cb95 100644 --- a/src/pae/ieee802_1x_kay.c +++ b/src/pae/ieee802_1x_kay.c @@ -3056,13 +3056,17 @@ ieee802_1x_kay_init(struct ieee802_1x_kay_ctx *ctx, enum macsec_policy policy, kay->macsec_replay_window = 0; kay->macsec_confidentiality = CONFIDENTIALITY_NONE; } else { - kay->macsec_capable = MACSEC_CAP_INTEG_AND_CONF_0_30_50; + if (secy_get_capability(kay, &kay->macsec_capable) < 0) + kay->macsec_capable = MACSEC_CAP_INTEG_AND_CONF_0_30_50; kay->macsec_desired = TRUE; kay->macsec_protect = TRUE; kay->macsec_validate = Strict; kay->macsec_replay_protect = FALSE; kay->macsec_replay_window = 0; - kay->macsec_confidentiality = CONFIDENTIALITY_OFFSET_0; + if (kay->macsec_capable >= MACSEC_CAP_INTEG_AND_CONF) + kay->macsec_confidentiality = CONFIDENTIALITY_OFFSET_0; + else + kay->macsec_confidentiality = MACSEC_CAP_INTEGRITY; } wpa_printf(MSG_DEBUG, "KaY: state machine created"); @@ -3394,6 +3398,7 @@ ieee802_1x_kay_change_cipher_suite(struct ieee802_1x_kay *kay, unsigned int cs_index) { struct ieee802_1x_mka_participant *participant; + enum macsec_cap secy_cap; if (!kay) return -1; @@ -3412,6 +3417,12 @@ ieee802_1x_kay_change_cipher_suite(struct ieee802_1x_kay *kay, kay->macsec_csindex = cs_index; kay->macsec_capable = cipher_suite_tbl[kay->macsec_csindex].capable; + if (secy_get_capability(kay, &secy_cap) < 0) + return -3; + + if (kay->macsec_capable > secy_cap) + kay->macsec_capable = secy_cap; + participant = ieee802_1x_kay_get_principal_participant(kay); if (participant) { wpa_printf(MSG_INFO, "KaY: Cipher Suite changed"); diff --git a/src/pae/ieee802_1x_kay.h b/src/pae/ieee802_1x_kay.h index 1236923e02de..c6fa387ed1c1 100644 --- a/src/pae/ieee802_1x_kay.h +++ b/src/pae/ieee802_1x_kay.h @@ -134,6 +134,7 @@ struct ieee802_1x_kay_ctx { /* abstract wpa driver interface */ int (*macsec_init)(void *ctx, struct macsec_init_params *params); int (*macsec_deinit)(void *ctx); + int (*macsec_get_capability)(void *priv, enum macsec_cap *cap); int (*enable_protect_frames)(void *ctx, Boolean enabled); int (*set_replay_protect)(void *ctx, Boolean enabled, u32 window); int (*set_current_cipher_suite)(void *ctx, u64 cs); diff --git a/src/pae/ieee802_1x_secy_ops.c b/src/pae/ieee802_1x_secy_ops.c index 1511055de391..b57c670f4d0b 100644 --- a/src/pae/ieee802_1x_secy_ops.c +++ b/src/pae/ieee802_1x_secy_ops.c @@ -113,6 +113,26 @@ int secy_cp_control_enable_port(struct ieee802_1x_kay *kay, Boolean enabled) } +int secy_get_capability(struct ieee802_1x_kay *kay, enum macsec_cap *cap) +{ + struct ieee802_1x_kay_ctx *ops; + + if (!kay) { + wpa_printf(MSG_ERROR, "KaY: %s params invalid", __func__); + return -1; + } + + ops = kay->ctx; + if (!ops || !ops->macsec_get_capability) { + wpa_printf(MSG_ERROR, + "KaY: secy macsec_get_capability operation not supported"); + return -1; + } + + return ops->macsec_get_capability(ops->ctx, cap); +} + + int secy_get_receive_lowest_pn(struct ieee802_1x_kay *kay, struct receive_sa *rxsa) { diff --git a/src/pae/ieee802_1x_secy_ops.h b/src/pae/ieee802_1x_secy_ops.h index 120ca3c7e883..bfd5737725e9 100644 --- a/src/pae/ieee802_1x_secy_ops.h +++ b/src/pae/ieee802_1x_secy_ops.h @@ -28,6 +28,7 @@ int secy_cp_control_confidentiality_offset(struct ieee802_1x_kay *kay, int secy_cp_control_enable_port(struct ieee802_1x_kay *kay, Boolean flag); /****** KaY -> SecY *******/ +int secy_get_capability(struct ieee802_1x_kay *kay, enum macsec_cap *cap); int secy_get_receive_lowest_pn(struct ieee802_1x_kay *kay, struct receive_sa *rxsa); int secy_get_transmit_next_pn(struct ieee802_1x_kay *kay, diff --git a/wpa_supplicant/driver_i.h b/wpa_supplicant/driver_i.h index 5eff8c1ebbb8..93a970e19a06 100644 --- a/wpa_supplicant/driver_i.h +++ b/wpa_supplicant/driver_i.h @@ -715,6 +715,15 @@ static inline int wpa_drv_macsec_deinit(struct wpa_supplicant *wpa_s) return wpa_s->driver->macsec_deinit(wpa_s->drv_priv); } +static inline int wpa_drv_macsec_get_capability(struct wpa_supplicant *wpa_s, + enum macsec_cap *cap) +{ + if (!wpa_s->driver->macsec_get_capability) + return -1; + + return wpa_s->driver->macsec_get_capability(wpa_s->drv_priv, cap); +} + static inline int wpa_drv_enable_protect_frames(struct wpa_supplicant *wpa_s, Boolean enabled) { diff --git a/wpa_supplicant/wpas_kay.c b/wpa_supplicant/wpas_kay.c index ad0312f03650..5e14d2cc2d46 100644 --- a/wpa_supplicant/wpas_kay.c +++ b/wpa_supplicant/wpas_kay.c @@ -38,6 +38,12 @@ static int wpas_macsec_deinit(void *priv) } +static int wpas_macsec_get_capability(void *priv, enum macsec_cap *cap) +{ + return wpa_drv_macsec_get_capability(priv, cap); +} + + static int wpas_enable_protect_frames(void *wpa_s, Boolean enabled) { return wpa_drv_enable_protect_frames(wpa_s, enabled); @@ -180,6 +186,7 @@ int ieee802_1x_alloc_kay_sm(struct wpa_supplicant *wpa_s, struct wpa_ssid *ssid) kay_ctx->macsec_init = wpas_macsec_init; kay_ctx->macsec_deinit = wpas_macsec_deinit; + kay_ctx->macsec_get_capability = wpas_macsec_get_capability; kay_ctx->enable_protect_frames = wpas_enable_protect_frames; kay_ctx->set_replay_protect = wpas_set_replay_protect; kay_ctx->set_current_cipher_suite = wpas_set_current_cipher_suite;
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- src/drivers/driver.h | 2 ++ src/pae/ieee802_1x_kay.c | 15 +++++++++++++-- src/pae/ieee802_1x_kay.h | 1 + src/pae/ieee802_1x_secy_ops.c | 20 ++++++++++++++++++++ src/pae/ieee802_1x_secy_ops.h | 1 + wpa_supplicant/driver_i.h | 9 +++++++++ wpa_supplicant/wpas_kay.c | 7 +++++++ 7 files changed, 53 insertions(+), 2 deletions(-)