A couple years back we went through the kernel an automatically
converted size calculations to use struct_size() instead. The
struct_size() calculation is protected against integer overflows.
However it does not make sense to use the result from struct_size()
for additional math operations as that would negate any safeness.
Fixes: 1f3b69b6b939 ("i2c: mux: Use struct_size() in devm_kzalloc()")
Signed-off-by: Dan Carpenter <[email protected]>
---
drivers/i2c/i2c-mux.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 774507b54b57..313904be5f3b 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -243,9 +243,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
int (*deselect)(struct i2c_mux_core *, u32))
{
struct i2c_mux_core *muxc;
+ size_t mux_size;
- muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
- + sizeof_priv, GFP_KERNEL);
+ mux_size = struct_size(muxc, adapter, max_adapters);
+ muxc = devm_kzalloc(dev, size_add(mux_size, sizeof_priv), GFP_KERNEL);
if (!muxc)
return NULL;
if (sizeof_priv)
--
2.35.1
On Thu, Sep 15, 2022 at 02:30:58PM +0300, Dan Carpenter wrote:
> A couple years back we went through the kernel an automatically
> converted size calculations to use struct_size() instead. The
> struct_size() calculation is protected against integer overflows.
>
> However it does not make sense to use the result from struct_size()
> for additional math operations as that would negate any safeness.
Right; there most be a couple more similar cases out there. I'll
look for them and fix them. Thanks!
>
> Fixes: 1f3b69b6b939 ("i2c: mux: Use struct_size() in devm_kzalloc()")
> Signed-off-by: Dan Carpenter <[email protected]>
Reviewed-by: Gustavo A. R. Silva <[email protected]>
--
Gustavo
> ---
> drivers/i2c/i2c-mux.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 774507b54b57..313904be5f3b 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -243,9 +243,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
> int (*deselect)(struct i2c_mux_core *, u32))
> {
> struct i2c_mux_core *muxc;
> + size_t mux_size;
>
> - muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
> - + sizeof_priv, GFP_KERNEL);
> + mux_size = struct_size(muxc, adapter, max_adapters);
> + muxc = devm_kzalloc(dev, size_add(mux_size, sizeof_priv), GFP_KERNEL);
> if (!muxc)
> return NULL;
> if (sizeof_priv)
> --
> 2.35.1
>
Hi Dan,
2022-09-15 at 13:30, Dan Carpenter wrote:
> A couple years back we went through the kernel an automatically
> converted size calculations to use struct_size() instead. The
> struct_size() calculation is protected against integer overflows.
>
> However it does not make sense to use the result from struct_size()
> for additional math operations as that would negate any safeness.
Indeed. Nice catch!
> Fixes: 1f3b69b6b939 ("i2c: mux: Use struct_size() in devm_kzalloc()")
> Signed-off-by: Dan Carpenter <[email protected]>
Acked-by: Peter Rosin <[email protected]>
Cheers,
Peter
> ---
> drivers/i2c/i2c-mux.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 774507b54b57..313904be5f3b 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -243,9 +243,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
> int (*deselect)(struct i2c_mux_core *, u32))
> {
> struct i2c_mux_core *muxc;
> + size_t mux_size;
>
> - muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
> - + sizeof_priv, GFP_KERNEL);
> + mux_size = struct_size(muxc, adapter, max_adapters);
> + muxc = devm_kzalloc(dev, size_add(mux_size, sizeof_priv), GFP_KERNEL);
> if (!muxc)
> return NULL;
> if (sizeof_priv)
On Thu, Sep 15, 2022 at 02:51:21PM +0100, Gustavo A. R. Silva wrote:
> On Thu, Sep 15, 2022 at 02:30:58PM +0300, Dan Carpenter wrote:
> > A couple years back we went through the kernel an automatically
> > converted size calculations to use struct_size() instead. The
> > struct_size() calculation is protected against integer overflows.
> >
> > However it does not make sense to use the result from struct_size()
> > for additional math operations as that would negate any safeness.
>
> Right; there most be a couple more similar cases out there. I'll
> look for them and fix them. Thanks!
>
That thought occured to me too. :P The main problem with that theory
is that sometimes people use struct_size() for readability instead of
just for checking for integer overflows. Also there are some places
which check for integer overflows manually before doing the math. So
this code is not perfect.
It would probaby be useful to mark passed data as explicitly unsafe for
integer overflows. Smatch already tracks user data. And if the user
data has been capped to an unknown value. But this would be a
completely separate flag which says that "this value came from
size_add/mul()".
regards,
dan carpenter
drivers/char/tpm/eventlog/tpm2.c:57 tpm2_bios_measurements_start() warn: using integer overflow function 'size_add()' for math
drivers/i2c/i2c-mux.c:248 i2c_mux_alloc() warn: using integer overflow function 'size_add()' for math
drivers/infiniband/hw/qib/qib_user_sdma.c:949 qib_user_sdma_queue_pkts() warn: using integer overflow function 'size_add()' for math
drivers/spi/spi.c:3320 spi_replace_transfers() warn: using integer overflow function 'size_add()' for math
drivers/gpu/drm/msm/msm_gem_submit.c:35 submit_create() warn: using integer overflow function 'size_add()' for math
drivers/cxl/pmem.c:151 cxl_pmem_set_config_data() warn: using integer overflow function 'size_add()' for math
drivers/md/dm-stats.c:295 dm_stats_create() warn: using integer overflow function 'size_add()' for math
drivers/md/dm-ioctl.c:1607 retrieve_deps() warn: using integer overflow function 'size_add()' for math
drivers/remoteproc/remoteproc_core.c:527 rproc_handle_vdev() warn: using integer overflow function 'size_add()' for math
drivers/rpmsg/qcom_glink_native.c:984 qcom_glink_handle_intent() warn: using integer overflow function 'size_add()' for math
drivers/net/ethernet/qlogic/qed/qed_ll2.c:1610 qed_ll2_establish_connection() warn: using integer overflow function 'size_add()' for math
drivers/net/ethernet/chelsio/cxgb4/sge.c:2551 cxgb4_ethofld_send_flowc() warn: using integer overflow function 'size_add()' for math
drivers/net/ethernet/intel/ice/ice_flex_pipe.c:2070 ice_pkg_buf_reserve_section() warn: using integer overflow function 'size_mul()' for math
drivers/net/ethernet/intel/ice/ice_switch.c:2562 ice_add_marker_act() warn: using integer overflow function 'size_add()' for math
drivers/net/ethernet/intel/ice/ice_switch.c:2567 ice_add_marker_act() warn: using integer overflow function 'size_add()' for math
drivers/net/ethernet/intel/ice/ice_switch.c:5478 ice_dummy_packet_add_vlan() warn: using integer overflow function 'size_mul()' for math
drivers/net/ethernet/intel/ice/ice_switch.c:5501 ice_dummy_packet_add_vlan() warn: using integer overflow function 'size_mul()' for math
drivers/gpio/gpiolib.c:4261 gpiod_get_array() warn: using integer overflow function 'size_add()' for math
drivers/gpio/gpiolib.c:4261 gpiod_get_array() warn: using integer overflow function 'size_add()' for math
fs/ntfs3/xattr.c:26 unpacked_ea_size() warn: using integer overflow function 'size_add()' for math
fs/ntfs3/xattr.c:291 ntfs_set_ea() warn: using integer overflow function 'size_add()' for math
io_uring/io_uring.c:2477 rings_size() warn: using integer overflow function 'size_add()' for math
kernel/module/sysfs.c:83 add_sect_attrs() warn: using integer overflow function 'size_add()' for math
kernel/irq/generic-chip.c:310 __irq_alloc_domain_generic_chips() warn: using integer overflow function 'size_add()' for math
kernel/irq/generic-chip.c:310 __irq_alloc_domain_generic_chips() warn: using integer overflow function 'size_add()' for math
kernel/dma/swiotlb.c:355 swiotlb_init_remap() warn: using integer overflow function 'size_mul()' for math
kernel/dma/swiotlb.c:476 swiotlb_exit() warn: using integer overflow function 'size_mul()' for math
sound/soc/qcom/qdsp6/q6apm.c:103 audioreach_graph_mgmt_cmd() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:458 audioreach_populate_graph() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:501 audioreach_alloc_graph_pkt() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:502 audioreach_alloc_graph_pkt() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:503 audioreach_alloc_graph_pkt() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:505 audioreach_alloc_graph_pkt() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:506 audioreach_alloc_graph_pkt() warn: using integer overflow function 'size_add()' for math
sound/soc/qcom/qdsp6/audioreach.c:842 audioreach_pcm_set_media_format() warn: using integer overflow function 'size_add()' for math
net/wireless/scan.c:765 cfg80211_scan_6ghz() warn: using integer overflow function 'size_add()' for math
net/tls/tls_sw.c:1486 tls_decrypt_sg() warn: using integer overflow function 'size_add()' for math
net/bridge/br_multicast.c:2770 br_ip6_multicast_mld2_report() warn: using integer overflow function 'size_add()' for math
net/bluetooth/hci_codec.c:153 hci_read_supported_codecs() warn: using integer overflow function 'size_mul()' for math
net/bluetooth/hci_codec.c:165 hci_read_supported_codecs() warn: using integer overflow function 'size_mul()' for math
net/bluetooth/hci_codec.c:172 hci_read_supported_codecs() warn: using integer overflow function 'size_mul()' for math
net/bluetooth/hci_codec.c:220 hci_read_supported_codecs_v2() warn: using integer overflow function 'size_mul()' for math
net/bluetooth/hci_codec.c:232 hci_read_supported_codecs_v2() warn: using integer overflow function 'size_mul()' for math
net/bluetooth/hci_codec.c:239 hci_read_supported_codecs_v2() warn: using integer overflow function 'size_mul()' for math
lib/stackdepot.c:125 depot_alloc_stack() warn: using integer overflow function 'size_add()' for math
mm/percpu.c:2444 pcpu_alloc_alloc_info() warn: using integer overflow function 'size_add()' for math
On Thu, Sep 15, 2022 at 02:30:58PM +0300, Dan Carpenter wrote:
> A couple years back we went through the kernel an automatically
> converted size calculations to use struct_size() instead. The
> struct_size() calculation is protected against integer overflows.
>
> However it does not make sense to use the result from struct_size()
> for additional math operations as that would negate any safeness.
>
> Fixes: 1f3b69b6b939 ("i2c: mux: Use struct_size() in devm_kzalloc()")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/i2c/i2c-mux.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 774507b54b57..313904be5f3b 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -243,9 +243,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
> int (*deselect)(struct i2c_mux_core *, u32))
> {
> struct i2c_mux_core *muxc;
> + size_t mux_size;
>
> - muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
> - + sizeof_priv, GFP_KERNEL);
> + mux_size = struct_size(muxc, adapter, max_adapters);
> + muxc = devm_kzalloc(dev, size_add(mux_size, sizeof_priv), GFP_KERNEL);
> if (!muxc)
> return NULL;
> if (sizeof_priv)
The new variable makes it more readable, but beyond that, do you see any
reason not to just directly compose the calls?
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 774507b54b57..6c481cde6517 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -244,8 +244,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
{
struct i2c_mux_core *muxc;
- muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
- + sizeof_priv, GFP_KERNEL);
+ muxc = devm_kzalloc(dev,
+ size_add(struct_size(muxc, adapter, max_adapters),
+ sizeof_priv),
+ GFP_KERNEL);
if (!muxc)
return NULL;
if (sizeof_priv)
> --
> 2.35.1
>
--
Kees Cook
On Fri, Sep 16, 2022 at 01:07:25AM -0700, Kees Cook wrote:
> On Thu, Sep 15, 2022 at 05:09:45PM +0300, Dan Carpenter wrote:
> > It would probaby be useful to mark passed data as explicitly unsafe for
> > integer overflows. Smatch already tracks user data. And if the user
> > data has been capped to an unknown value. But this would be a
> > completely separate flag which says that "this value came from
> > size_add/mul()".
>
> I really want a __must_check_type(size_t) attribute or something for
> functions, so we can get a subset of -Wconversion warnings, etc.
>
I have a list of these. Attached.
> > drivers/char/tpm/eventlog/tpm2.c:57 tpm2_bios_measurements_start() warn: using integer overflow function 'size_add()' for math
> > [...]
> > drivers/net/ethernet/intel/ice/ice_flex_pipe.c:2070 ice_pkg_buf_reserve_section() warn: using integer overflow function 'size_mul()' for math
>
> I see size_add() and size_mul() here. I would have expected some
> size_sub() opportunities too? Did nothing pop out?
I didn't look at size_sub(). I'll add it to the mix and report back on
Monday.
regards,
dan carpenter
On Thu, Sep 15, 2022 at 05:09:45PM +0300, Dan Carpenter wrote:
> It would probaby be useful to mark passed data as explicitly unsafe for
> integer overflows. Smatch already tracks user data. And if the user
> data has been capped to an unknown value. But this would be a
> completely separate flag which says that "this value came from
> size_add/mul()".
I really want a __must_check_type(size_t) attribute or something for
functions, so we can get a subset of -Wconversion warnings, etc.
> drivers/char/tpm/eventlog/tpm2.c:57 tpm2_bios_measurements_start() warn: using integer overflow function 'size_add()' for math
> [...]
> drivers/net/ethernet/intel/ice/ice_flex_pipe.c:2070 ice_pkg_buf_reserve_section() warn: using integer overflow function 'size_mul()' for math
I see size_add() and size_mul() here. I would have expected some
size_sub() opportunities too? Did nothing pop out?
--
Kees Cook
On Fri, Sep 16, 2022 at 01:01:42AM -0700, Kees Cook wrote:
> > diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> > index 774507b54b57..313904be5f3b 100644
> > --- a/drivers/i2c/i2c-mux.c
> > +++ b/drivers/i2c/i2c-mux.c
> > @@ -243,9 +243,10 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
> > int (*deselect)(struct i2c_mux_core *, u32))
> > {
> > struct i2c_mux_core *muxc;
> > + size_t mux_size;
> >
> > - muxc = devm_kzalloc(dev, struct_size(muxc, adapter, max_adapters)
> > - + sizeof_priv, GFP_KERNEL);
> > + mux_size = struct_size(muxc, adapter, max_adapters);
> > + muxc = devm_kzalloc(dev, size_add(mux_size, sizeof_priv), GFP_KERNEL);
> > if (!muxc)
> > return NULL;
> > if (sizeof_priv)
>
> The new variable makes it more readable, but beyond that, do you see any
> reason not to just directly compose the calls?
>
You could do that too.
You pointed this out in your other email but the one thing that people
have to be careful of when assigning struct_size() is that the
"mux_size" variable has to be size_t.
The math in submit_create() from drivers/gpu/drm/msm/msm_gem_submit.c
is so terribly unreadable. It works but it's so ugly. Unfortunately,
I'm the person who wrote it.
regards,
dan carpenter
On Fri, Sep 16, 2022 at 11:23:25AM +0300, Dan Carpenter wrote:
> [...]
> net/ipv6/mcast.c:450 ip6_mc_source() saving 'size_add' to type 'int'
Interesting! Are you able to report the consumer? e.g. I think a bunch
of these would be fixed by:
diff --git a/net/core/sock.c b/net/core/sock.c
index 4cb957d934a2..f004c4d411e3 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2552,10 +2552,11 @@ struct sk_buff *sock_omalloc(struct sock *sk, unsigned long size,
/*
* Allocate a memory block from the socket's option memory buffer.
*/
-void *sock_kmalloc(struct sock *sk, int size, gfp_t priority)
+void *sock_kmalloc(struct sock *sk, size_t size, gfp_t priority)
{
- if ((unsigned int)size <= sysctl_optmem_max &&
- atomic_read(&sk->sk_omem_alloc) + size < sysctl_optmem_max) {
+ if (size > INT_MAX || size > sysctl_optmem_max)
+ return NULL;
+ if (atomic_read(&sk->sk_omem_alloc) < sysctl_optmem_max - size) {
void *mem;
/* First do the add, to avoid the race if kmalloc
* might sleep.
--
Kees Cook
On Fri, Sep 16, 2022 at 06:31:45AM -0700, Kees Cook wrote:
> On Fri, Sep 16, 2022 at 11:23:25AM +0300, Dan Carpenter wrote:
> > [...]
> > net/ipv6/mcast.c:450 ip6_mc_source() saving 'size_add' to type 'int'
>
> Interesting! Are you able to report the consumer? e.g. I think a bunch
> of these would be fixed by:
>
Are you asking if I can add "passed to sock_kmalloc()" to the report?
It's possible but it's kind of a headache the way this code is written.
When you pass a function to another function in Smatch:
frob(size_add());
Then Smatch creates a fake assignment: "frob(fake_assign = size_add());"
and parses that instead. So this check only looks at the
"fake_assign = size_add();" assignment.
Attached.
regards,
dan carpenter
> > The new variable makes it more readable, but beyond that, do you see any
> > reason not to just directly compose the calls?
> >
>
> You could do that too.
>
> You pointed this out in your other email but the one thing that people
> have to be careful of when assigning struct_size() is that the
> "mux_size" variable has to be size_t.
>
> The math in submit_create() from drivers/gpu/drm/msm/msm_gem_submit.c
> is so terribly unreadable. It works but it's so ugly. Unfortunately,
> I'm the person who wrote it.
I can't parse from that if the patch in question is okay or needs a
respin? Could you kindly enlighten me?
On Fri, Sep 16, 2022 at 05:55:55PM +0300, Dan Carpenter wrote:
> On Fri, Sep 16, 2022 at 06:31:45AM -0700, Kees Cook wrote:
> > On Fri, Sep 16, 2022 at 11:23:25AM +0300, Dan Carpenter wrote:
> > > [...]
> > > net/ipv6/mcast.c:450 ip6_mc_source() saving 'size_add' to type 'int'
> >
> > Interesting! Are you able to report the consumer? e.g. I think a bunch
> > of these would be fixed by:
> >
>
> Are you asking if I can add "passed to sock_kmalloc()" to the report?
Yeah.
> It's possible but it's kind of a headache the way this code is written.
Okay, no worries -- I was curious if it would be "easy". I can happily
just spit out the source line.
--
Kees Cook
On Fri, Sep 16, 2022 at 08:31:58PM +0100, Wolfram Sang wrote:
>
> > > The new variable makes it more readable, but beyond that, do you see any
> > > reason not to just directly compose the calls?
> > >
> >
> > You could do that too.
> >
> > You pointed this out in your other email but the one thing that people
> > have to be careful of when assigning struct_size() is that the
> > "mux_size" variable has to be size_t.
> >
> > The math in submit_create() from drivers/gpu/drm/msm/msm_gem_submit.c
> > is so terribly unreadable. It works but it's so ugly. Unfortunately,
> > I'm the person who wrote it.
>
> I can't parse from that if the patch in question is okay or needs a
> respin? Could you kindly enlighten me?
>
It doesn't need a respin. We were just discussing related bugs with the
integer overflow safe functions.
regards,
dan carpenter
On Thu, Sep 15, 2022 at 02:30:58PM +0300, Dan Carpenter wrote:
> A couple years back we went through the kernel an automatically
> converted size calculations to use struct_size() instead. The
> struct_size() calculation is protected against integer overflows.
>
> However it does not make sense to use the result from struct_size()
> for additional math operations as that would negate any safeness.
>
> Fixes: 1f3b69b6b939 ("i2c: mux: Use struct_size() in devm_kzalloc()")
> Signed-off-by: Dan Carpenter <[email protected]>
Applied to for-current, thanks!