* [bug report] clk: visconti: Add support common clock driver and reset driver
@ 2022-01-11 8:14 Dan Carpenter
2022-01-20 20:55 ` Stephen Boyd
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-01-11 8:14 UTC (permalink / raw)
To: nobuhiro1.iwamatsu; +Cc: linux-clk
Hello Nobuhiro Iwamatsu,
The patch b4cbe606dc36: "clk: visconti: Add support common clock
driver and reset driver" from Oct 25, 2021, leads to the following
Smatch static checker warning:
drivers/clk/visconti/clkc.c:150 visconti_clk_register_gates()
warn: always true condition '(clks[i]->rs_id >= 0) => (0-255 >= 0)'
drivers/clk/visconti/clkc.c
124 int visconti_clk_register_gates(struct visconti_clk_provider *ctx,
125 const struct visconti_clk_gate_table *clks,
126 int num_gate,
127 const struct visconti_reset_data *reset,
128 spinlock_t *lock)
129 {
130 struct device *dev = ctx->dev;
131 int i;
132
133 for (i = 0; i < num_gate; i++) {
134 const char *parent_div_name = clks[i].parent_data[0].name;
135 struct clk_parent_data *pdata;
136 u32 rson_offset, rsoff_offset;
137 struct clk_hw *gate_clk;
138 struct clk_hw *div_clk;
139 char *dev_name;
140 u8 rs_idx;
141
142 pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
143 if (!pdata)
144 return -ENOMEM;
145
146 dev_name = devm_kasprintf(dev, GFP_KERNEL, "%s_div", clks[i].name);
147 if (!dev_name)
148 return -ENOMEM;
149
--> 150 if (clks[i].rs_id >= 0) {
This is a u8. But it does get set to -1 for PISYSTEM in clkc-tmpv770x.c
> drivers/clk/visconti/clkc-tmpv770x.c
> 175 /* PISYSTEM */
> 176 { TMPV770X_CLK_WRCK, "wrck",
> 177 clks_parent_data, ARRAY_SIZE(clks_parent_data),
> 178 0, 0x68, 0x168, 9, 32,
> 179 -1, }, /* No reset */
151 rson_offset = reset[clks[i].rs_id].rson_offset;
152 rsoff_offset = reset[clks[i].rs_id].rsoff_offset;
153 rs_idx = reset[clks[i].rs_id].rs_idx;
154 } else {
155 rson_offset = rsoff_offset = rs_idx = -1;
All these variables are unsigned but they're set to -1. clks[i].rs_idx
is also unsigned.
156 }
157
158 div_clk = devm_clk_hw_register_fixed_factor(dev,
159 dev_name,
160 parent_div_name,
161 0, 1,
162 clks[i].div);
163 if (IS_ERR(div_clk))
164 return PTR_ERR(div_clk);
165
166 gate_clk = visconti_clk_register_gate(dev,
167 clks[i].name,
168 dev_name,
169 ctx->regmap,
170 &clks[i],
171 rson_offset,
172 rsoff_offset,
173 rs_idx,
174 lock);
175 if (IS_ERR(gate_clk)) {
176 dev_err(dev, "%s: failed to register clock %s\n",
177 __func__, clks[i].name);
178 return PTR_ERR(gate_clk);
179 }
180
181 ctx->clk_data.hws[clks[i].id] = gate_clk;
182 }
183
184 return 0;
185 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* [bug report] clk: visconti: Add support common clock driver and reset driver
@ 2025-12-09 13:05 Dan Carpenter
2025-12-10 7:36 ` Nathan Chancellor
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2025-12-09 13:05 UTC (permalink / raw)
To: linux-hardening, Nobuhiro Iwamatsu, llvm
Hello Kernel Hardenning developers,
Commit b4cbe606dc36 ("clk: visconti: Add support common clock driver
and reset driver") from Oct 25, 2021 (linux-next), leads to the
question:
drivers/clk/visconti/clkc.c
187 struct visconti_clk_provider *visconti_init_clk(struct device *dev,
188 struct regmap *regmap,
189 unsigned long nr_clks)
190 {
191 struct visconti_clk_provider *ctx;
192 int i;
193
194 ctx = devm_kzalloc(dev, struct_size(ctx, clk_data.hws, nr_clks), GFP_KERNEL);
195 if (!ctx)
196 return ERR_PTR(-ENOMEM);
197
198 for (i = 0; i < nr_clks; ++i)
--> 199 ctx->clk_data.hws[i] = ERR_PTR(-ENOENT);
200 ctx->clk_data.num = nr_clks;
ctx->clk_data.hws[] is __counted_by() ctx->clk_data.num. Don't we have to
set the .num before we fill initialize the array? Or does the checker
code allow us to access the array when the counted by variable is zero?
I seem to remember this used to be a common false positive with the counted
by checking.
201
202 ctx->dev = dev;
203 ctx->regmap = regmap;
204
205 return ctx;
206 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] clk: visconti: Add support common clock driver and reset driver
2025-12-09 13:05 Dan Carpenter
@ 2025-12-10 7:36 ` Nathan Chancellor
2025-12-10 7:53 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Nathan Chancellor @ 2025-12-10 7:36 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-hardening, Nobuhiro Iwamatsu, llvm
Hey Dan,
On Tue, Dec 09, 2025 at 04:05:58PM +0300, Dan Carpenter wrote:
> Hello Kernel Hardenning developers,
>
> Commit b4cbe606dc36 ("clk: visconti: Add support common clock driver
> and reset driver") from Oct 25, 2021 (linux-next), leads to the
> question:
>
> drivers/clk/visconti/clkc.c
> 187 struct visconti_clk_provider *visconti_init_clk(struct device *dev,
> 188 struct regmap *regmap,
> 189 unsigned long nr_clks)
> 190 {
> 191 struct visconti_clk_provider *ctx;
> 192 int i;
> 193
> 194 ctx = devm_kzalloc(dev, struct_size(ctx, clk_data.hws, nr_clks), GFP_KERNEL);
> 195 if (!ctx)
> 196 return ERR_PTR(-ENOMEM);
> 197
> 198 for (i = 0; i < nr_clks; ++i)
> --> 199 ctx->clk_data.hws[i] = ERR_PTR(-ENOENT);
> 200 ctx->clk_data.num = nr_clks;
>
> ctx->clk_data.hws[] is __counted_by() ctx->clk_data.num. Don't we have to
> set the .num before we fill initialize the array? Or does the checker
Yes, it looks like line 200 needs to be moved above line 198.
> code allow us to access the array when the counted by variable is zero?
Nope, I just had to fix an instance where num is one and hws[0] is being
accessed:
https://lore.kernel.org/20251124-exynos-clkout-fix-ubsan-bounds-error-v1-1-224a5282514b@kernel.org/
I suspect there will eventually be a report on real hardware when
CONFIG_UBSAN_BOUNDS is enabled and GCC 15 is used to build the running
kernel, which is the first version that supports __counted_by(). How did
you find this? A new smatch check? It would be nice if the compilers
could flag this but I guess it is probably harder to do this in a
generic way.
Cheers,
Nathan
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] clk: visconti: Add support common clock driver and reset driver
2025-12-10 7:36 ` Nathan Chancellor
@ 2025-12-10 7:53 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2025-12-10 7:53 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: linux-hardening, Nobuhiro Iwamatsu, llvm
On Wed, Dec 10, 2025 at 04:36:11PM +0900, Nathan Chancellor wrote:
> Hey Dan,
>
> On Tue, Dec 09, 2025 at 04:05:58PM +0300, Dan Carpenter wrote:
> > Hello Kernel Hardenning developers,
> >
> > Commit b4cbe606dc36 ("clk: visconti: Add support common clock driver
> > and reset driver") from Oct 25, 2021 (linux-next), leads to the
> > question:
> >
> > drivers/clk/visconti/clkc.c
> > 187 struct visconti_clk_provider *visconti_init_clk(struct device *dev,
> > 188 struct regmap *regmap,
> > 189 unsigned long nr_clks)
> > 190 {
> > 191 struct visconti_clk_provider *ctx;
> > 192 int i;
> > 193
> > 194 ctx = devm_kzalloc(dev, struct_size(ctx, clk_data.hws, nr_clks), GFP_KERNEL);
> > 195 if (!ctx)
> > 196 return ERR_PTR(-ENOMEM);
> > 197
> > 198 for (i = 0; i < nr_clks; ++i)
> > --> 199 ctx->clk_data.hws[i] = ERR_PTR(-ENOENT);
> > 200 ctx->clk_data.num = nr_clks;
> >
> > ctx->clk_data.hws[] is __counted_by() ctx->clk_data.num. Don't we have to
> > set the .num before we fill initialize the array? Or does the checker
>
> Yes, it looks like line 200 needs to be moved above line 198.
>
> > code allow us to access the array when the counted by variable is zero?
>
> Nope, I just had to fix an instance where num is one and hws[0] is being
> accessed:
>
> https://lore.kernel.org/20251124-exynos-clkout-fix-ubsan-bounds-error-v1-1-224a5282514b@kernel.org/
>
> I suspect there will eventually be a report on real hardware when
> CONFIG_UBSAN_BOUNDS is enabled and GCC 15 is used to build the running
> kernel, which is the first version that supports __counted_by(). How did
> you find this? A new smatch check?
Yeah, but it was a bug in my check. I need to add __counted_by()
support to Sparse. If I had that, it would easy enough to add a
warning for places which access the array before setting the counted
by variable.
regards,
carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-12-10 7:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-11 8:14 [bug report] clk: visconti: Add support common clock driver and reset driver Dan Carpenter
2022-01-20 20:55 ` Stephen Boyd
-- strict thread matches above, loose matches on Subject: below --
2025-12-09 13:05 Dan Carpenter
2025-12-10 7:36 ` Nathan Chancellor
2025-12-10 7:53 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.