diff mbox series

[06/16] mac80211_hwsim: make copying of ciphers safer by checking the length

Message ID 20190315163634.17315-7-luca@coelho.fi (mailing list archive)
State Rejected
Delegated to: Johannes Berg
Headers show
Series iwlwifi: updates intended for v5.1 2019-03-15 | expand

Commit Message

Luca Coelho March 15, 2019, 4:36 p.m. UTC
From: Luca Coelho <luciano.coelho@intel.com>

Make sure the length of the ciphers we are copying never exceeds the
space we have for storing them.  There is no risk of overcopying at
the moment, because we check n_params before, but this makes this
function safer in case someone changes something in the future.

Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/net/wireless/mac80211_hwsim.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Luca Coelho March 15, 2019, 4:40 p.m. UTC | #1
On Fri, 2019-03-15 at 18:36 +0200, Luca Coelho wrote:
> From: Luca Coelho <luciano.coelho@intel.com>
> 
> Make sure the length of the ciphers we are copying never exceeds the
> space we have for storing them.  There is no risk of overcopying at
> the moment, because we check n_params before, but this makes this
> function safer in case someone changes something in the future.
> 
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---

Ooops, this one should obviously not go through my tree.  Johannes, can
you please take it? I've removed from my tree and won't include it as
part of my pull-request.

I'll also check patchwork and make sure it's under Johannes' name.

--
Cheers,
Luca.
Johannes Berg April 8, 2019, 12:22 p.m. UTC | #2
On Fri, 2019-03-15 at 18:36 +0200, Luca Coelho wrote:
> From: Luca Coelho <luciano.coelho@intel.com>
> 
> Make sure the length of the ciphers we are copying never exceeds the
> space we have for storing them.  There is no risk of overcopying at
> the moment, because we check n_params before, but this makes this
> function safer in case someone changes something in the future.

I don't think this makes sense.

If anything, we should pass extack to mac80211_hwsim_new_radio() and do
the entire validation there, but doing the same thing twice in two
places just because of static checkers is useless.

johannes
diff mbox series

Patch

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 0838af04d681..809a75357113 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -3,7 +3,7 @@ 
  * Copyright (c) 2008, Jouni Malinen <j@w1.fi>
  * Copyright (c) 2011, Javier Lopez <jlopex@gmail.com>
  * Copyright (c) 2016 - 2017 Intel Deutschland GmbH
- * Copyright (C) 2018 Intel Corporation
+ * Copyright (C) 2018 - 2019 Intel Corporation
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -2776,10 +2776,14 @@  static int mac80211_hwsim_new_radio(struct genl_info *info,
 	hw->wiphy->n_iface_combinations = 1;
 
 	if (param->ciphers) {
-		memcpy(data->ciphers, param->ciphers,
-		       param->n_ciphers * sizeof(u32));
+		int ciphers_len = param->n_ciphers * sizeof(data->ciphers[0]);
+
+		if (WARN_ON_ONCE(ciphers_len > sizeof(data->ciphers)))
+			ciphers_len = sizeof(data->ciphers);
+
+		memcpy(data->ciphers, param->ciphers, ciphers_len);
 		hw->wiphy->cipher_suites = data->ciphers;
-		hw->wiphy->n_cipher_suites = param->n_ciphers;
+		hw->wiphy->n_cipher_suites = ciphers_len / sizeof(data->ciphers[0]);
 	}
 
 	INIT_DELAYED_WORK(&data->roc_start, hw_roc_start);