| Message ID | ce915d61-04bc-44fb-b450-35fcc9fc8831@moroto.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 74da77921333171766031ea213b11f1e650814f9 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/tcp_sigpool: Fix some off by one bugs | expand |
Hi Dan, On 10/31/23 09:51, Dan Carpenter wrote: > The "cpool_populated" variable is the number of elements in the cpool[] > array that have been populated. It is incremented in > tcp_sigpool_alloc_ahash() every time we populate a new element. > Unpopulated elements are NULL but if we have populated every element then > this code will read one element beyond the end of the array. > > Fixes: 8c73b26315aa ("net/tcp: Prepare tcp_md5sig_pool for TCP-AO") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Yeah, those are barriers for any issues in the caller-code, so that's not too critical but nice to have. Thanks for the patch! Reviewed-by: Dmitry Safonov <dima@arista.com> > --- > From static analysis and review. > > net/ipv4/tcp_sigpool.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/ipv4/tcp_sigpool.c b/net/ipv4/tcp_sigpool.c > index 65a8eaae2fec..55b310a722c7 100644 > --- a/net/ipv4/tcp_sigpool.c > +++ b/net/ipv4/tcp_sigpool.c > @@ -231,7 +231,7 @@ static void cpool_schedule_cleanup(struct kref *kref) > */ > void tcp_sigpool_release(unsigned int id) > { > - if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg)) > + if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg)) > return; > > /* slow-path */ > @@ -245,7 +245,7 @@ EXPORT_SYMBOL_GPL(tcp_sigpool_release); > */ > void tcp_sigpool_get(unsigned int id) > { > - if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg)) > + if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg)) > return; > kref_get(&cpool[id].kref); > } > @@ -256,7 +256,7 @@ int tcp_sigpool_start(unsigned int id, struct tcp_sigpool *c) __cond_acquires(RC > struct crypto_ahash *hash; > > rcu_read_lock_bh(); > - if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg)) { > + if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg)) { > rcu_read_unlock_bh(); > return -EINVAL; > } > @@ -301,7 +301,7 @@ EXPORT_SYMBOL_GPL(tcp_sigpool_end); > */ > size_t tcp_sigpool_algo(unsigned int id, char *buf, size_t buf_len) > { > - if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg)) > + if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg)) > return -EINVAL; > > return strscpy(buf, cpool[id].alg, buf_len); Thanks, Dmitry
On Tue, Oct 31, 2023 at 10:51 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > The "cpool_populated" variable is the number of elements in the cpool[] > array that have been populated. It is incremented in > tcp_sigpool_alloc_ahash() every time we populate a new element. > Unpopulated elements are NULL but if we have populated every element then > this code will read one element beyond the end of the array. > > Fixes: 8c73b26315aa ("net/tcp: Prepare tcp_md5sig_pool for TCP-AO") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > From static analysis and review. > Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 31 Oct 2023 12:51:09 +0300 you wrote: > The "cpool_populated" variable is the number of elements in the cpool[] > array that have been populated. It is incremented in > tcp_sigpool_alloc_ahash() every time we populate a new element. > Unpopulated elements are NULL but if we have populated every element then > this code will read one element beyond the end of the array. > > Fixes: 8c73b26315aa ("net/tcp: Prepare tcp_md5sig_pool for TCP-AO") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > [...] Here is the summary with links: - [net] net/tcp_sigpool: Fix some off by one bugs https://git.kernel.org/netdev/net/c/74da77921333 You are awesome, thank you!
diff --git a/net/ipv4/tcp_sigpool.c b/net/ipv4/tcp_sigpool.c index 65a8eaae2fec..55b310a722c7 100644 --- a/net/ipv4/tcp_sigpool.c +++ b/net/ipv4/tcp_sigpool.c @@ -231,7 +231,7 @@ static void cpool_schedule_cleanup(struct kref *kref) */ void tcp_sigpool_release(unsigned int id) { - if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg)) + if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg)) return; /* slow-path */ @@ -245,7 +245,7 @@ EXPORT_SYMBOL_GPL(tcp_sigpool_release); */ void tcp_sigpool_get(unsigned int id) { - if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg)) + if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg)) return; kref_get(&cpool[id].kref); } @@ -256,7 +256,7 @@ int tcp_sigpool_start(unsigned int id, struct tcp_sigpool *c) __cond_acquires(RC struct crypto_ahash *hash; rcu_read_lock_bh(); - if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg)) { + if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg)) { rcu_read_unlock_bh(); return -EINVAL; } @@ -301,7 +301,7 @@ EXPORT_SYMBOL_GPL(tcp_sigpool_end); */ size_t tcp_sigpool_algo(unsigned int id, char *buf, size_t buf_len) { - if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg)) + if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg)) return -EINVAL; return strscpy(buf, cpool[id].alg, buf_len);
The "cpool_populated" variable is the number of elements in the cpool[] array that have been populated. It is incremented in tcp_sigpool_alloc_ahash() every time we populate a new element. Unpopulated elements are NULL but if we have populated every element then this code will read one element beyond the end of the array. Fixes: 8c73b26315aa ("net/tcp: Prepare tcp_md5sig_pool for TCP-AO") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- From static analysis and review. net/ipv4/tcp_sigpool.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)