[PATCH][next] dax: remove redundant assignment to variable rc
The variable rc is being assigned an value and then is being re-assigned a new value in the next statement. The assignment is redundant and can be removed. Cleans up clang scan build warning: drivers/dax/bus.c:1207:2: warning: Value stored to 'rc' is never read [deadcode.DeadStores] Signed-off-by: Colin Ian King --- drivers/dax/bus.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c index 797e1ebff299..f758afbf8f09 100644 --- a/drivers/dax/bus.c +++ b/drivers/dax/bus.c @@ -1204,7 +1204,6 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr, if (rc) return rc; - rc = -ENXIO; rc = down_write_killable(_region_rwsem); if (rc) return rc; -- 2.39.2
[PATCH][next] fsdax: remove redundant variable 'error'
The variable 'error' is being assigned a value that is never read,
the assignment and the variable and redundant and can be removed.
Cleans up clang scan build warning:
fs/dax.c:1880:10: warning: Although the value stored to 'error' is
used in the enclosing expression, the value is never actually read
from 'error' [deadcode.DeadStores]
Signed-off-by: Colin Ian King
---
fs/dax.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index 2ababb89918d..cb36c6746fc4 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1830,7 +1830,6 @@ static vm_fault_t dax_iomap_pmd_fault(struct vm_fault
*vmf, pfn_t *pfnp,
vm_fault_t ret = VM_FAULT_FALLBACK;
pgoff_t max_pgoff;
void *entry;
- int error;
if (vmf->flags & FAULT_FLAG_WRITE)
iter.flags |= IOMAP_WRITE;
@@ -1877,7 +1876,7 @@ static vm_fault_t dax_iomap_pmd_fault(struct vm_fault
*vmf, pfn_t *pfnp,
}
iter.pos = (loff_t)xas.xa_index << PAGE_SHIFT;
- while ((error = iomap_iter(, ops)) > 0) {
+ while (iomap_iter(, ops) > 0) {
if (iomap_length() < PMD_SIZE)
continue; /* actually breaks out of the loop */
--
2.39.2
[PATCH] nvdimm/region: Fix spelling mistake "memergion" -> "memregion"
There is a spelling mistake in a dev_warn message. Fix it.
Signed-off-by: Colin Ian King
---
drivers/nvdimm/region_devs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 83dbf398ea84..8f5274b04348 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -80,7 +80,7 @@ static int nd_region_invalidate_memregion(struct nd_region
*nd_region)
if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) {
dev_warn(
_region->dev,
- "Bypassing cpu_cache_invalidate_memergion() for
testing!\n");
+ "Bypassing cpu_cache_invalidate_memregion() for
testing!\n");
goto out;
} else {
dev_err(_region->dev,
--
2.38.1
Re: [PATCH] perf/x86: Fix integer overflow when left shifting an integer more than 32 bits
On 20/04/2021 16:31, Peter Zijlstra wrote:
> On Tue, Apr 20, 2021 at 05:03:03PM +0200, Peter Zijlstra wrote:
>> On Tue, Apr 20, 2021 at 03:29:07PM +0100, Colin King wrote:
>>> From: Colin Ian King
>>>
>>> The 64 bit value read from MSR_ARCH_PERFMON_FIXED_CTR_CTRL is being
>>> bit-wise masked with the value (0x03 << i*4). However, the shifted value
>>> is evaluated using 32 bit arithmetic, so will overflow when i > 8.
>>> Fix this by making 0x03 a ULL so that the shift is performed using
>>> 64 bit arithmetic.
>>>
>>> Addresses-Coverity: ("Unintentional integer overflow")
>>
>> Strange tag that, also inaccurate, wide shifts are UB and don't behave
>> consistently.
>>
>> As is, we've not had hardware with that many fixed counters, but yes,
>> worth fixing I suppose.
>
> Patch now reads:
>
> ---
> Subject: perf/x86: Allow for 8 From: Colin Ian King
> Date: Tue, 20 Apr 2021 15:29:07 +0100
>
> From: Colin Ian King
>
> The 64 bit value read from MSR_ARCH_PERFMON_FIXED_CTR_CTRL is being
> bit-wise masked with the value (0x03 << i*4). However, the shifted value
> is evaluated using 32 bit arithmetic, so will UB when i > 8. Fix this
> by making 0x03 a ULL so that the shift is performed using 64 bit
> arithmetic.
>
> This makes the arithmetic internally consistent and preparers for the
> day when hardware provides 8
> Signed-off-by: Colin Ian King
> Signed-off-by: Peter Zijlstra (Intel)
> Link:
> https://lkml.kernel.org/r/20210420142907.382417-1-colin.k...@canonical.com
> ---
> arch/x86/events/core.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -261,7 +261,7 @@ static bool check_hw_exists(void)
> for (i = 0; i < x86_pmu.num_counters_fixed; i++) {
> if (fixed_counter_disabled(i))
> continue;
> - if (val & (0x03 << i*4)) {
> + if (val & (0x03ULL << i*4)) {
> bios_fail = 1;
> val_fail = val;
> reg_fail = reg;
>
re: phy: nxp-c45: add driver for tja1103
Hi,
Static analysis with Coverity on linux-next has found a potential issue
in drivers/net/phy/nxp-c45-tja11xx.c, function nxp_c45_get_phase_shift.
The analysis by Coverity is as follows:
350 static u64 nxp_c45_get_phase_shift(u64 phase_offset_raw)
351 {
352/* The delay in degree phase is 73.8 + phase_offset_raw * 0.9.
353 * To avoid floating point operations we'll multiply by 10
354 * and get 1 decimal point precision.
355 */
356phase_offset_raw *= 10;
Operands don't affect result (CONSTANT_EXPRESSION_RESULT)
result_independent_of_operands: phase_offset_raw is always assigned 0.
Did you intend to negate the value of phase_offset_raw instead of
assigning it 0? This occurs as the value assigned by "-".
357phase_offset_raw -= phase_offset_raw;
358return div_u64(phase_offset_raw, 9);
359 }
phase_offset_raw -= phase_offset_raw results in phase_offset_raw being
zero, I don't think that was the intent.
Colin
Re: [PATCH][next] can: etas_es58x: Fix potential null pointer dereference on pointer cf
On 15/04/2021 10:03, Marc Kleine-Budde wrote:
> On 15.04.2021 09:55:35, Colin King wrote:
>> From: Colin Ian King
>>
>> The pointer cf is being null checked earlier in the code, however the
>> update of the rx_bytes statistics is dereferencing cf without null
>> checking cf. Fix this by moving the statement into the following code
>> block that has a null cf check.
>>
>> Addresses-Coverity: ("Dereference after null check")
>> Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN
>> USB interfaces")
>> Signed-off-by: Colin Ian King
>
> A somewhat different fix is already in net-next/master
>
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=e2b1e4b532abdd39bfb7313146153815e370d60c
+1 on that
>
> Marc
>
Re: [PATCH][next] KEYS: trusted: Fix missing null return from kzalloc call
On 12/04/2021 17:48, James Bottomley wrote:
> On Mon, 2021-04-12 at 17:01 +0100, Colin King wrote:
>> From: Colin Ian King
>>
>> The kzalloc call can return null with the GFP_KERNEL flag so
>> add a null check and exit via a new error exit label. Use the
>> same exit error label for another error path too.
>>
>> Addresses-Coverity: ("Dereference null return value")
>> Fixes: 830027e2cb55 ("KEYS: trusted: Add generic trusted keys
>> framework")
>> Signed-off-by: Colin Ian King
>> ---
>> security/keys/trusted-keys/trusted_core.c | 6 --
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/security/keys/trusted-keys/trusted_core.c
>> b/security/keys/trusted-keys/trusted_core.c
>> index ec3a066a4b42..90774793f0b1 100644
>> --- a/security/keys/trusted-keys/trusted_core.c
>> +++ b/security/keys/trusted-keys/trusted_core.c
>> @@ -116,11 +116,13 @@ static struct trusted_key_payload
>> *trusted_payload_alloc(struct key *key)
>>
>> ret = key_payload_reserve(key, sizeof(*p));
>> if (ret < 0)
>> -return p;
>> +goto err;
>> p = kzalloc(sizeof(*p), GFP_KERNEL);
>> +if (!p)
>> +goto err;
>>
>> p->migratable = migratable;
>> -
>> +err:
>> return p;
>
> This is clearly a code migration bug in
>
> commit 251c85bd106099e6f388a89e88e12d14de2c9cda
> Author: Sumit Garg
> Date: Mon Mar 1 18:41:24 2021 +0530
>
> KEYS: trusted: Add generic trusted keys framework
>
> Which has for addition to trusted_core.c:
>
> +static struct trusted_key_payload *trusted_payload_alloc(struct key
> *key)
> +{
> + struct trusted_key_payload *p = NULL;
> + int ret;
> +
> + ret = key_payload_reserve(key, sizeof(*p));
> + if (ret < 0)
> + return p;
> + p = kzalloc(sizeof(*p), GFP_KERNEL);
> +
> + p->migratable = migratable;
> +
> + return p;
> +}
>
> And for trusted_tpm1.c:
>
> -static struct trusted_key_payload *trusted_payload_alloc(struct key
> *key)
> -{
> - struct trusted_key_payload *p = NULL;
> - int ret;
> -
> - ret = key_payload_reserve(key, sizeof *p);
> - if (ret < 0)
> - return p;
> - p = kzalloc(sizeof *p, GFP_KERNEL);
> - if (p)
> - p->migratable = 1; /* migratable by default */
> - return p;
> -}
>
> The trusted_tpm1.c code was correct and we got this bug introduced by
> what should have been a simple cut and paste ... how did that happen?
> And therefore, how safe is the rest of the extraction into
> trusted_core.c?
>
fortunately it gets caught by static analysis, but it does make me also
concerned about what else has changed and how this gets through review.
> James
>
>
Re: [PATCH] xfs: fix return of uninitialized value in variable error
On 09/04/2021 15:28, Brian Foster wrote:
> On Fri, Apr 09, 2021 at 03:18:34PM +0100, Colin King wrote:
>> From: Colin Ian King
>>
>> A previous commit removed a call to xfs_attr3_leaf_read that
>> assigned an error return code to variable error. We now have
>> a few early error return paths to label 'out' that return
>> error if error is set; however error now is uninitialized
>> so potentially garbage is being returned. Fix this by setting
>> error to zero to restore the original behaviour where error
>> was zero at the label 'restart'.
>>
>> Addresses-Coverity: ("Uninitialized scalar variable")
>> Fixes: 07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
>> Signed-off-by: Colin Ian King
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 472b3039eabb..902e5f7e6642 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -928,6 +928,7 @@ xfs_attr_node_addname(
>> * Search to see if name already exists, and get back a pointer
>> * to where it should go.
>> */
>> +error = 0;
>> retval = xfs_attr_node_hasname(args, );
>> if (retval != -ENOATTR && retval != -EEXIST)
>> goto out;
>
> I think it would be nicer to initialize at the top of the function as
> opposed to try and "preserve" historical behavior, but that nit aside:
I did think about that, but this fix does ensure it's zero'd for each
iteration rather than just the once, so it should catch any code changes
later on that may loop back to this point were error is non-zero.
>
> Reviewed-by: Brian Foster
>
>> --
>> 2.30.2
>>
>
cnic: issue with double assignment to ictx->xstorm_st_context.common.flags
Hi, Analysis of linux with Coverity has detected an issue with the assignment of ictx->xstorm_st_context.common.fla in drivers/net/ethernet/broadcom/cnic.c in function cnic_setup_bnx2x_ctx. This was introduced a while back with the following commit: commit 619c5cb6885b936c44ae1422ef805b69c6291485 Author: Vlad Zolotarov Date: Tue Jun 14 14:33:44 2011 +0300 New 7.0 FW: bnx2x, cnic, bnx2i, bnx2fc The static analysis is as follows: 1761ictx->xstorm_st_context.common.flags = Unused value (UNUSED_VALUE)assigned_value: Assigning value 1 to ictx->xstorm_st_context.common.flags here, but that stored value is overwritten before it can be used. 17621 << XSTORM_COMMON_CONTEXT_SECTION_PHYSQ_INITIALIZED_SHIFT; 1763ictx->xstorm_st_context.common.flags = value_overwrite: Overwriting previous write to ictx->xstorm_st_context.common.flags with value from port << 1. 1764port << XSTORM_COMMON_CONTEXT_SECTION_PBF_PORT_SHIFT; 1765 1766ictx->tstorm_st_context.iscsi.hdr_bytes_2_fetch = ISCSI_HEADER_SIZE; The re-assignment of ictx->xstorm_st_context.common.flags in line 1763 is overwriting the value assigned on line 1761. Should the = operator on the re-assignment be an |= operator instead? Colin
Re: [PATCH][next] mlxsw: spectrum_router: remove redundant initialization of variable force
On 29/03/2021 08:13, Ido Schimmel wrote:
> On Sat, Mar 27, 2021 at 10:33:34PM +, Colin King wrote:
>> From: Colin Ian King
>>
>> The variable force is being initialized with a value that is
>> never read and it is being updated later with a new value. The
>> initialization is redundant and can be removed.
>>
>> Addresses-Coverity: ("Unused value")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> index 6ccaa194733b..ff240e3c29f7 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>> @@ -5059,7 +5059,7 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct mlxsw_sp
>> *mlxsw_sp,
>> {
>> u16 bucket_index = info->nh_res_bucket->bucket_index;
>> struct netlink_ext_ack *extack = info->extack;
>> -bool force = info->nh_res_bucket->force;
>> +bool force;
>
> Actually, there is a bug to be fixed here:
>
> ```
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> index 6ccaa194733b..41259c0004d1 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> @@ -5068,8 +5068,9 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct mlxsw_sp
> *mlxsw_sp,
> /* No point in trying an atomic replacement if the idle timer interval
> * is smaller than the interval in which we query and clear activity.
> */
> - force = info->nh_res_bucket->idle_timer_ms <
> - MLXSW_SP_NH_GRP_ACTIVITY_UPDATE_INTERVAL;
> + if (!force && info->nh_res_bucket->idle_timer_ms <
> + MLXSW_SP_NH_GRP_ACTIVITY_UPDATE_INTERVAL)
> + force = true;
>
> adj_index = nh->nhgi->adj_index + bucket_index;
> err = mlxsw_sp_nexthop_update(mlxsw_sp, adj_index, nh, force,
> ratr_pl);
> ```
>
> We should only fallback to a non-atomic replacement when the current
> replacement is atomic and the idle timer is too short.
>
> We currently ignore the value of 'force'. This means that a non-atomic
> replacement ('force' is true) can be made atomic if idle timer is larger
> than 1 second.
>
> Colin, do you mind if I submit it formally as a fix later this week? I
> want to run it through our usual process. Will mention you in
> Reported-by, obviously.
Sure. Good idea.
>
>> char ratr_pl_new[MLXSW_REG_RATR_LEN];
>> char ratr_pl[MLXSW_REG_RATR_LEN];
>> u32 adj_index;
>> --
>> 2.30.2
>>
Re: [PATCH] clk: uniphier: Fix potential infinite loop
On 09/04/2021 07:46, Masahiro Yamada wrote:
> On Thu, Apr 8, 2021 at 12:25 AM Colin King wrote:
>>
>> From: Colin Ian King
>>
>> The for-loop iterates with a u8 loop counter i and compares this
>> with the loop upper limit of num_parents that is an int type.
>> There is a potential infinite loop if num_parents is larger than
>> the u8 loop counter. Fix this by making the loop counter the same
>> type as num_parents.
>>
>> Addresses-Coverity: ("Infinite loop")
>> Fixes: 734d82f4a678 ("clk: uniphier: add core support code for UniPhier
>> clock driver")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/clk/uniphier/clk-uniphier-mux.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/uniphier/clk-uniphier-mux.c
>> b/drivers/clk/uniphier/clk-uniphier-mux.c
>> index 462c84321b2d..ce219e0d2a85 100644
>> --- a/drivers/clk/uniphier/clk-uniphier-mux.c
>> +++ b/drivers/clk/uniphier/clk-uniphier-mux.c
>> @@ -34,7 +34,7 @@ static u8 uniphier_clk_mux_get_parent(struct clk_hw *hw)
>> int num_parents = clk_hw_get_num_parents(hw);
>> int ret;
>> unsigned int val;
>> - u8 i;
>> + int i;
>>
>> ret = regmap_read(mux->regmap, mux->reg, );
>> if (ret)
>> --
>> 2.30.2
>>
>
> clk_hw_get_num_parents() returns 'unsigned int', so
> I think 'num_parents' should also have been 'unsigned int'.
>
> Maybe, the loop counter 'i' also should be 'unsigned int' then?
>
>
Good point. I'll send a V2.
re: drm/i915/selftests: Prepare gtt tests for obj->mm.lock removal
Hi,
Static analysis with Coverity on Linux-next has detected a potential
issue with the following commit:
commit 480ae79537b28f30ef6e07b7de69a9ae2599daa7
Author: Maarten Lankhorst
Date: Tue Mar 23 16:50:49 2021 +0100
drm/i915/selftests: Prepare gtt tests for obj->mm.lock removal
The analysis by Coverity is as follows:
145 static int igt_ppgtt_alloc(void *arg)
146 {
147struct drm_i915_private *dev_priv = arg;
148struct i915_ppgtt *ppgtt;
1. var_decl: Declaring variable ww without initializer.
149struct i915_gem_ww_ctx ww;
150u64 size, last, limit;
151int err = 0;
152
153/* Allocate a ppggt and try to fill the entire range */
154
2. Condition !(dev_priv->__info.ppgtt_type != INTEL_PPGTT_NONE),
taking false branch.
155if (!HAS_PPGTT(dev_priv))
156return 0;
157
158ppgtt = i915_ppgtt_create(_priv->gt);
3. Condition IS_ERR(ppgtt), taking false branch.
159if (IS_ERR(ppgtt))
160return PTR_ERR(ppgtt);
161
4. Condition !ppgtt->vm.allocate_va_range, taking true branch.
162if (!ppgtt->vm.allocate_va_range)
5. Jumping to label err_ppgtt_cleanup.
163goto err_ppgtt_cleanup;
164
165/*
166 * While we only allocate the page tables here and so we could
167 * address a much larger GTT than we could actually fit into
168 * RAM, a practical limit is the amount of physical pages in
the system.
169 * This should ensure that we do not run into the oomkiller
during
170 * the test and take down the machine wilfully.
171 */
172limit = totalram_pages() << PAGE_SHIFT;
173limit = min(ppgtt->vm.total, limit);
174
175i915_gem_ww_ctx_init(, false);
176retry:
177err = i915_vm_lock_objects(>vm, );
178if (err)
179goto err_ppgtt_cleanup;
180
181/* Check we can allocate the entire range */
182for (size = 4096; size <= limit; size <<= 2) {
183struct i915_vm_pt_stash stash = {};
184
185err = i915_vm_alloc_pt_stash(>vm, , size);
186if (err)
187goto err_ppgtt_cleanup;
188
189err = i915_vm_pin_pt_stash(>vm, );
190if (err) {
191i915_vm_free_pt_stash(>vm, );
192goto err_ppgtt_cleanup;
193}
194
195ppgtt->vm.allocate_va_range(>vm, , 0, size);
196cond_resched();
197
198ppgtt->vm.clear_range(>vm, 0, size);
199
200i915_vm_free_pt_stash(>vm, );
201}
202
203/* Check we can incrementally allocate the entire range */
204for (last = 0, size = 4096; size <= limit; last = size, size
<<= 2) {
205struct i915_vm_pt_stash stash = {};
206
207err = i915_vm_alloc_pt_stash(>vm, , size
- last);
208if (err)
209goto err_ppgtt_cleanup;
210
211err = i915_vm_pin_pt_stash(>vm, );
212if (err) {
213i915_vm_free_pt_stash(>vm, );
214goto err_ppgtt_cleanup;
215}
216
217ppgtt->vm.allocate_va_range(>vm, ,
218last, size - last);
219cond_resched();
220
221i915_vm_free_pt_stash(>vm, );
222}
223
224 err_ppgtt_cleanup:
6. Condition err == -35, taking false branch.
225if (err == -EDEADLK) {
226err = i915_gem_ww_ctx_backoff();
227if (!err)
228goto retry;
229}
7. uninit_use_in_call: Using uninitialized value ww.contended when
calling i915_gem_ww_ctx_fini.
Uninitialized pointer read (UNINIT)
8. uninit_use_in_call: Using uninitialized value ww.ctx.acquired when
calling i915_gem_ww_ctx_fini.
230i915_gem_ww_ctx_fini();
231
232i915_vm_put(>vm);
233return err;
234 }
Coverity is reporting use of uninitialized values in (lines 230. Not
sure what the best fix is for this, so I'm reporting this as a potential
issue.
Colin
re: ALSA: control - add layer registration routines
Hi,
Static analysis on linux-next with Coverity has detected a potential
issue in the following commit:
commit 3f0638a0333bfdd0549985aa620f2ab69737af47
Author: Jaroslav Kysela
Date: Wed Mar 17 18:29:41 2021 +0100
ALSA: control - add layer registration routines
The static analysis is as follows:
2072 void snd_ctl_disconnect_layer(struct snd_ctl_layer_ops *lops)
2073 {
2074struct snd_ctl_layer_ops *lops2, *prev_lops2;
2075
2076down_write(_ctl_layer_rwsem);
assignment: Assigning: prev_lops2 = NULL.
2077for (lops2 = snd_ctl_layer, prev_lops2 = NULL; lops2; lops2
= lops2->next)
2078if (lops2 == lops) {
null: At condition prev_lops2, the value of prev_lops2 must be NULL.
dead_error_condition: The condition !prev_lops2 must be true.
2079if (!prev_lops2)
2080snd_ctl_layer = lops->next;
2081else
'Constant' variable guards dead code (DEADCODE) dead_error_line:
Execution cannot reach this statement: prev_lops2->next = lops->next;.
Local variable prev_lops2 is assigned only once, to a constant
value, making it effectively constant throughout its scope. If this is
not the intent, examine the logic to see if there is a missing
assignment that would make prev_lops2 not remain constant.
2082prev_lops2->next = lops->next;
2083break;
2084}
2085up_write(_ctl_layer_rwsem);
2086 }
I couldn't quite figure out the original intent of the prev_lops use, so
I'd thought I'd report this issue as the code does look incorrect.
Colin
Re: [PATCH] mm/page_alloc: Add a bulk page allocator -fix -fix
On 30/03/2021 12:48, Mel Gorman wrote: > Colin Ian King reported the following problem (slightly edited) > > Author: Mel Gorman > Date: Mon Mar 29 11:12:24 2021 +1100 > > mm/page_alloc: add a bulk page allocator > > ... > > Static analysis on linux-next with Coverity has found a potential > uninitialized variable issue in function __alloc_pages_bulk with > the following commit: > > ... > > Uninitialized scalar variable (UNINIT) > 15. uninit_use_in_call: Using uninitialized value alloc_flags when > calling prepare_alloc_pages. > > 5056if (!prepare_alloc_pages(gfp, 0, preferred_nid, nodemask, > , _gfp, _flags)) > > The problem is that prepare_alloc_flags only updates alloc_flags > which must have a valid initial value. The appropriate initial value is > ALLOC_WMARK_LOW to avoid the bulk allocator pushing a zone below the low > watermark without waking kswapd assuming the GFP mask allows kswapd to > be woken. > > This is a second fix to the mmotm patch > mm-page_alloc-add-a-bulk-page-allocator.patch . It will cause a mild conflict > with a later patch due to renaming of an adjacent variable that is trivially > resolved. I can post a full series with the fixes merged if that is preferred. > > Signed-off-by: Mel Gorman > --- > mm/page_alloc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 92d55f80c289..dabef0b910c9 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -4990,7 +4990,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int > preferred_nid, > struct list_head *pcp_list; > struct alloc_context ac; > gfp_t alloc_gfp; > - unsigned int alloc_flags; > + unsigned int alloc_flags = ALLOC_WMARK_LOW; > int allocated = 0; > > if (WARN_ON_ONCE(nr_pages <= 0)) > Thanks Mel, that definitely fixes the issue. Reviewed-by: Colin Ian King
re: mm/page_alloc: add a bulk page allocator
Hi,
Static analysis on linux-next with Coverity has found a potential
uninitialized variable issue in function __alloc_pages_bulk with the
following commit:
commit b0e0a469733fa571ddd8fe147247c9561b51b2da
Author: Mel Gorman
Date: Mon Mar 29 11:12:24 2021 +1100
mm/page_alloc: add a bulk page allocator
The analysis is as follows:
5023 unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
5024nodemask_t *nodemask, int nr_pages,
5025struct list_head *page_list,
5026struct page **page_array)
5027 {
5028struct page *page;
5029unsigned long flags;
5030struct zone *zone;
5031struct zoneref *z;
5032struct per_cpu_pages *pcp;
5033struct list_head *pcp_list;
5034struct alloc_context ac;
5035gfp_t alloc_gfp;
1. var_decl: Declaring variable alloc_flags without initializer.
5036unsigned int alloc_flags;
5037int nr_populated = 0;
5038
2. Condition !!(nr_pages <= 0), taking false branch.
5039if (unlikely(nr_pages <= 0))
5040return 0;
5041
5042/*
5043 * Skip populated array elements to determine if any pages need
5044 * to be allocated before disabling IRQs.
5045 */
3. Condition page_array, taking true branch.
4. Condition page_array[nr_populated], taking true branch.
5. Condition nr_populated < nr_pages, taking true branch.
7. Condition page_array, taking true branch.
8. Condition page_array[nr_populated], taking true branch.
9. Condition nr_populated < nr_pages, taking true branch.
11. Condition page_array, taking true branch.
12. Condition page_array[nr_populated], taking true branch.
13. Condition nr_populated < nr_pages, taking false branch.
5046while (page_array && page_array[nr_populated] &&
nr_populated < nr_pages)
6. Jumping back to the beginning of the loop.
10. Jumping back to the beginning of the loop.
5047nr_populated++;
5048
5049/* Use the single page allocator for one page. */
14. Condition nr_pages - nr_populated == 1, taking false branch.
5050if (nr_pages - nr_populated == 1)
5051goto failed;
5052
5053/* May set ALLOC_NOFRAGMENT, fragmentation will return 1
page. */
5054gfp &= gfp_allowed_mask;
5055alloc_gfp = gfp;
Uninitialized scalar variable (UNINIT)
15. uninit_use_in_call: Using uninitialized value alloc_flags when
calling prepare_alloc_pages.
5056if (!prepare_alloc_pages(gfp, 0, preferred_nid, nodemask,
, _gfp, _flags))
5057return 0;
And in prepare_alloc_pages():
4957 static inline bool prepare_alloc_pages(gfp_t gfp_mask, unsigned int
order,
4958int preferred_nid, nodemask_t *nodemask,
4959struct alloc_context *ac, gfp_t *alloc_gfp,
4960unsigned int *alloc_flags)
4961 {
4962ac->highest_zoneidx = gfp_zone(gfp_mask);
4963ac->zonelist = node_zonelist(preferred_nid, gfp_mask);
4964ac->nodemask = nodemask;
4965ac->migratetype = gfp_migratetype(gfp_mask);
4966
1. Condition cpusets_enabled(), taking false branch.
4967if (cpusets_enabled()) {
4968*alloc_gfp |= __GFP_HARDWALL;
4969/*
4970 * When we are in the interrupt context, it is
irrelevant
4971 * to the current task context. It means that any
node ok.
4972 */
4973if (!in_interrupt() && !ac->nodemask)
4974ac->nodemask = _current_mems_allowed;
4975else
4976*alloc_flags |= ALLOC_CPUSET;
4977}
4978
4979fs_reclaim_acquire(gfp_mask);
4980fs_reclaim_release(gfp_mask);
4981
2. Condition gfp_mask & 1024U /* (gfp_t)1024U */, taking true branch.
4982might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
4983
3. Condition should_fail_alloc_page(gfp_mask, order), taking false
branch.
4984if (should_fail_alloc_page(gfp_mask, order))
4985return false;
4986
4. read_value: Reading value *alloc_flags when calling
gfp_to_alloc_flags_cma.
4987*alloc_flags = gfp_to_alloc_flags_cma(gfp_mask, *alloc_flags);
And in call gfp_to_alloc_flags_cma():
in /mm/page_alloc.c
3853 static inline unsigned int gfp_to_alloc_flags_cma(gfp_t gfp_mask,
3854 unsigned int
alloc_flags)
3855 {
3856#ifdef CONFIG_CMA
1. Condition gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE, taking
true branch.
3857if (gfp_migratetype(gfp_mask) == MIGRATE_MOVABLE)
2. read_value: Reading value alloc_flags.
3858alloc_flags |= ALLOC_CMA;
3859#endif
3860return alloc_flags;
3861 }
So alloc_flags in gfp_to_alloc_flags_cma is being updated with the |=
operator and we managed to get to this path with uninitialized
re: drm/ttm: switch to per device LRU lock
Hi,
Static analysis with Coverity in linux-next has detected an issue in
drivers/gpu/drm/ttm/ttm_bo.c with the follow commit:
commit a1f091f8ef2b680a5184db065527612247cb4cae
Author: Christian König
Date: Tue Oct 6 17:26:42 2020 +0200
drm/ttm: switch to per device LRU lock
Instead of having a global lock for potentially less contention.
The analysis is as follows:
617 int ttm_mem_evict_first(struct ttm_device *bdev,
618struct ttm_resource_manager *man,
619const struct ttm_place *place,
620struct ttm_operation_ctx *ctx,
621struct ww_acquire_ctx *ticket)
622 {
1. assign_zero: Assigning: bo = NULL.
623struct ttm_buffer_object *bo = NULL, *busy_bo = NULL;
624bool locked = false;
625unsigned i;
626int ret;
627
Explicit null dereferenced (FORWARD_NULL)2. var_deref_op:
Dereferencing null pointer bo.
628spin_lock(>bdev->lru_lock);
629for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
The spin_lock on bo is dereferencing a null bo pointer.
Colin
Re: [PATCH] x86/kprobes: Remove dead code
On 24/03/2021 17:36, Muhammad Usama Anjum wrote:
> The condition in switch statement `opcode & 0xf0` cannot evaluate to
> 0xff. So this case statement will never execute. Remove it.
>
> Fixes: 6256e668b7 ("x86/kprobes: Use int3 instead of debug trap for
> single-step")
> Signed-off-by: Muhammad Usama Anjum
> ---
> arch/x86/kernel/kprobes/core.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 89d9f26785c7..3b7bcc077020 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -177,9 +177,6 @@ int can_boost(struct insn *insn, void *addr)
> case 0xf0:
> /* clear and set flags are boostable */
> return (opcode == 0xf5 || (0xf7 < opcode && opcode < 0xfe));
> - case 0xff:
> - /* indirect jmp is boostable */
> - return X86_MODRM_REG(insn->modrm.bytes[0]) == 4;
> default:
> /* CS override prefix and call are not boostable */
> return (opcode != 0x2e && opcode != 0x9a);
>
The 0xff case was added with some form of intention to be executed so I
suspect removing it is not an appropriate fix.
Re: [PATCH][next] staging: rtl8188eu: Fix null pointer dereference on free_netdev call
On 24/03/2021 16:11, Martin Kaiser wrote:
> Hello Colin,
>
> Thus wrote Colin King (colin.k...@canonical.com):
>
>> From: Colin Ian King
>
>> An unregister_netdev call checks if pnetdev is null, hence a later
>> call to free_netdev can potentially be passing a null pointer, causing
>> a null pointer dereference. Avoid this by adding a null pointer check
>> on pnetdev before calling free_netdev.
>
>> Fixes: 1665c8fdffbb ("staging: rtl8188eu: use netdev routines for private
>> data")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/staging/rtl8188eu/os_dep/usb_intf.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>> diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
>> b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
>> index 518e9feb3f46..91a3d34a1050 100644
>> --- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
>> +++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
>> @@ -446,7 +446,8 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
>> pr_debug("+r871xu_dev_remove, hw_init_completed=%d\n",
>> if1->hw_init_completed);
>> rtw_free_drv_sw(if1);
>> -free_netdev(pnetdev);
>> +if (pnetdev)
>> +free_netdev(pnetdev);
>> }
>
>> static int rtw_drv_init(struct usb_interface *pusb_intf, const struct
>> usb_device_id *pdid)
>> --
>> 2.30.2
>
> you're right. I removed the NULL check that was part of rtw_free_netdev.
> Sorry for the mistake and thanks for your fix.
Thank static analysis :-)
>
> Reviewed-by: Martin Kaiser
>
> Best regards,
> Martin
>
re: x86/kprobes: Use int3 instead of debug trap for single-step
Hi,
Static analysis on linux-next using Coverity has detected an issue in
the following commit:
commit 6256e668b7af9d81472e03c6a171630c08f8858a
Author: Masami Hiramatsu
Date: Wed Mar 3 00:25:46 2021 +0900
x86/kprobes: Use int3 instead of debug trap for single-step
The analysis is as follows:
160switch (opcode & 0xf0) {
161case 0x60:
162/* can't boost "bound" */
163return (opcode != 0x62);
164case 0x70:
165return 0; /* can't boost conditional jump */
166case 0x90:
167return opcode != 0x9a; /* can't boost call far */
168case 0xc0:
169/* can't boost software-interruptions */
170return (0xc1 < opcode && opcode < 0xcc) || opcode ==
0xcf;
171case 0xd0:
172/* can boost AA* and XLAT */
173return (opcode == 0xd4 || opcode == 0xd5 || opcode ==
0xd7);
174case 0xe0:
175/* can boost in/out and absolute jmps */
176return ((opcode & 0x04) || opcode == 0xea);
177case 0xf0:
178/* clear and set flags are boostable */
179return (opcode == 0xf5 || (0xf7 < opcode && opcode <
0xfe));
dead_error_condition: The switch governing value opcode & 0xf0 cannot
be 255.
undefined (#1 of 1): Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement: case 255:
180case 0xff:
181/* indirect jmp is boostable */
182 return X86_MODRM_REG(insn->modrm.bytes[0]) == 4;
the case 0xff statement can never be reached because the switch
statement is acting on opcode & 0xf0.
Colin
Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call
On 19/03/2021 15:54, Dan Schatzberg wrote: > On Thu, Mar 18, 2021 at 03:16:26PM +, Colin King wrote: >> From: Colin Ian King >> >> The 3rd argument to alloc_workqueue should be the max_active count, >> however currently it is the lo->lo_number that is intended for the >> loop%d number. Fix this by adding in the missing max_active count. >> > > Thanks for catching this Colin. I'm fairly new to kernel development. > Is there some tool I could have run locally to catch this? > I'm using Coverity to find these issues. There is a free version [1], but the one I use is licensed and has the scan level turned up quite high to catch more issues than the free service. Refs: [1] https://scan.coverity.com/projects/linux-next-weekly-scan Colin
Re: [PATCH][next] loop: Fix missing max_active argument in alloc_workqueue call
On 18/03/2021 20:12, Jens Axboe wrote: > On 3/18/21 9:16 AM, Colin King wrote: >> From: Colin Ian King >> >> The 3rd argument to alloc_workqueue should be the max_active count, >> however currently it is the lo->lo_number that is intended for the >> loop%d number. Fix this by adding in the missing max_active count. > > Dan, please fold this (or something similar) in when you're redoing the > series. > Appreciate this fix being picked up. Are we going to lose the SoB? Colin
Re: [PATCH][next] soc: xilinx: vcu: remove deadcode on null divider check
On 11/02/2021 19:05, Stephen Boyd wrote:
> Quoting Michael Tretter (2021-02-10 23:39:06)
>> On Wed, 10 Feb 2021 19:28:18 -0800, Stephen Boyd wrote:
>>> Quoting Colin King (2021-02-10 10:49:38)
>>>> From: Colin Ian King
>>>>
>>>> The pointer 'divider' has previously been null checked followed by
>>>> a return, hence the subsequent null check is redundant deadcode
>>>> that can be removed. Clean up the code and remove it.
>>>>
>>>> Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for
>>>> output clocks")
>>>> Signed-off-by: Colin Ian King
>>>> ---
>>>> drivers/clk/xilinx/xlnx_vcu.c | 3 ---
>>>> 1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
>>>> index d66b1315114e..607936d7a413 100644
>>>> --- a/drivers/clk/xilinx/xlnx_vcu.c
>>>> +++ b/drivers/clk/xilinx/xlnx_vcu.c
>>>> @@ -512,9 +512,6 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw
>>>> *hw)
>>>>
>>>> mux = clk_hw_get_parent(divider);
>>>> clk_hw_unregister_mux(mux);
>>>> - if (!divider)
>>>> - return;
>>>> -
>>>
>>> This code is pretty confusing. Waiting for m.tret...@pengutronix.de to
>>> reply
>>
>> Can you elaborate what you find confusing about this code. I would gladly try
>> to clarify and improve the code.
>
> The fact that pointers are being checked and then bailing out of the
> function early, vs. doing something if the pointer is non-NULL.
>
>>
>> What happens here is that the driver registers a mux -> divider -> gate chain
>> for each output clock, but only stores the gate clock. When unregistering the
>> clocks, the driver starts at the gate and walks up to the mux while
>> unregistering the clocks.
>>
OK, so I think I understand this better, should the order of
unregisteration be as follows:
diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
index d66b1315114e..66bac8421460 100644
--- a/drivers/clk/xilinx/xlnx_vcu.c
+++ b/drivers/clk/xilinx/xlnx_vcu.c
@@ -511,11 +511,11 @@ static void xvcu_clk_hw_unregister_leaf(struct
clk_hw *hw)
return;
mux = clk_hw_get_parent(divider);
- clk_hw_unregister_mux(mux);
- if (!divider)
+ clk_hw_unregister_mux(divider);
+ if (!mux)
return;
- clk_hw_unregister_divider(divider);
+ clk_hw_unregister_divider(mux);
Re: pinctrl: core: Handling pinmux and pinconf separately
On 12/03/2021 12:45, Andy Shevchenko wrote: > On Thu, Mar 11, 2021 at 1:26 PM Colin Ian King > wrote: >> On 11/03/2021 11:16, Michal Simek wrote: >>> On 3/11/21 11:57 AM, Colin Ian King wrote: > >>>> For the PIN_MAP_TYPE_CONFIGS_PIN and PIN_MAP_TYPE_CONFIGS_GROUP >>>> setting->type cases the loop can break out with ret not being set. Since >>>> ret has not been initialized it the ret < 0 check is checking against an >>>> uninitialized value. >>>> >>>> I was not sure if the PIN_MAP_TYPE_CONFIGS_PIN and >>>> PIN_MAP_TYPE_CONFIGS_GROUP cases should be setting ret and if so, what >>>> the value of ret should be set to (is it an error condition or not?). Or >>>> should ret be initialized to 0 or a default error value at the start of >>>> the function. >>>> >>>> Hence I'm reporting this issue. >>> >>> What about this? Is this passing static analysis? >> >> It will take me 2 hours to re-run the analysis, but from eyeballing the >> code I think the assignments will fix this. > > It surprises me that tools in the 21st century can't run on a subset > of the data. > > Had you filed a bug to the Coverity team that they will provide a way > to rerun analysis on a subset of the data? It can. However I need to keep copies of the entire build to do this and I build many different kernels (hence lots of storage required) and rarely do minor change + rebuilds, so I don't cater for this in my test build environment. > >
Re: [PATCH] pinctrl: core: Set ret to 0 when group is skipped
On 12/03/2021 07:31, Michal Simek wrote:
> Static analyzer tool found that the ret variable is not initialized but
> code expects ret value >=0 when pinconf is skipped in the first pinmux
> loop. The same expectation is for pinmux in a pinconf loop.
> That's why initialize ret to 0 to avoid uninitialized ret value in first
> loop or reusing ret value from first loop in second.
>
> Addresses-Coverity: ("Uninitialized variables")
> Signed-off-by: Michal Simek
> CC: Colin Ian King
> CC: Dan Carpenter
> ---
>
> drivers/pinctrl/core.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index f5c32d2a3c91..136c323d855e 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1266,6 +1266,7 @@ static int pinctrl_commit_state(struct pinctrl *p,
> struct pinctrl_state *state)
> break;
> case PIN_MAP_TYPE_CONFIGS_PIN:
> case PIN_MAP_TYPE_CONFIGS_GROUP:
> + ret = 0;
> break;
> default:
> ret = -EINVAL;
> @@ -1284,6 +1285,7 @@ static int pinctrl_commit_state(struct pinctrl *p,
> struct pinctrl_state *state)
> list_for_each_entry(setting, >settings, node) {
> switch (setting->type) {
> case PIN_MAP_TYPE_MUX_GROUP:
> + ret = 0;
> break;
> case PIN_MAP_TYPE_CONFIGS_PIN:
> case PIN_MAP_TYPE_CONFIGS_GROUP:
>
Thanks Michal,
Reviewed-by: Colin Ian King
Re: [PATCH][next] nvmem: core: Fix unintentional sign extension issue
On 11/03/2021 17:12, Doug Anderson wrote:
> Hi,
>
> On Thu, Mar 11, 2021 at 1:53 AM Colin King wrote:
>>
>> From: Colin Ian King
>>
>> The shifting of the u8 integer buf[3] by 24 bits to the left will
>> be promoted to a 32 bit signed int and then sign-extended to a
>> u64. In the event that the top bit of buf[3] is set then all
>> then all the upper 32 bits of the u64 end up as also being set
>> because of the sign-extension. Fix this by casting buf[i] to
>> a u64 before the shift.
>>
>> Addresses-Coverity: ("Unintended sign extension")
>> Fixes: 097eb1136ebb ("nvmem: core: Add functions to make number reading
>> easy")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/nvmem/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Thanks! I had only tested the "u64" version to read smaller data and
> store it in a u64. From my understanding of C rules, without your
> patch it would have been even worse than just a sign extension though,
> right? Shifting "buf[i]" by more than 32 bits would just not have
> worked right.
yep, that's correct, I forgot to mention that issue too :-/
>
> In any case:
>
> Reviewed-by: Douglas Anderson
>
Re: pinctrl: core: Handling pinmux and pinconf separately
On 11/03/2021 11:28, Michal Simek wrote:
>
>
> On 3/11/21 12:24 PM, Colin Ian King wrote:
>> On 11/03/2021 11:16, Michal Simek wrote:
>>>
>>>
>>> On 3/11/21 11:57 AM, Colin Ian King wrote:
>>>> Hi,
>>>>
>>>> Static analysis on linux-next with Coverity has found a potential issue
>>>> in drivers/pinctrl/core.c with the following commit:
>>>>
>>>> commit 0952b7ec1614abf232e921aac0cc2bca8e60e162
>>>> Author: Michal Simek
>>>> Date: Wed Mar 10 09:16:54 2021 +0100
>>>>
>>>> pinctrl: core: Handling pinmux and pinconf separately
>>>>
>>>> The analysis is as follows:
>>>>
>>>> 1234 /**
>>>> 1235 * pinctrl_commit_state() - select/activate/program a pinctrl state
>>>> to HW
>>>> 1236 * @p: the pinctrl handle for the device that requests configuration
>>>> 1237 * @state: the state handle to select/activate/program
>>>> 1238 */
>>>> 1239 static int pinctrl_commit_state(struct pinctrl *p, struct
>>>> pinctrl_state *state)
>>>> 1240 {
>>>> 1241struct pinctrl_setting *setting, *setting2;
>>>> 1242struct pinctrl_state *old_state = p->state;
>>>>
>>>> 1. var_decl: Declaring variable ret without initializer.
>>>>
>>>> 1243int ret;
>>>> 1244
>>>>
>>>> 2. Condition p->state, taking true branch.
>>>>
>>>> 1245if (p->state) {
>>>> 1246/*
>>>> 1247 * For each pinmux setting in the old state, forget
>>>> SW's record
>>>> 1248 * of mux owner for that pingroup. Any pingroups
>>>> which are
>>>> 1249 * still owned by the new state will be re-acquired
>>>> by the call
>>>> 1250 * to pinmux_enable_setting() in the loop below.
>>>> 1251 */
>>>>
>>>> 3. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>> 4. Condition !(>node == >state->settings), taking true
>>>> branch.
>>>> 7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>> 8. Condition !(>node == >state->settings), taking true
>>>> branch.
>>>> 11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>> 12. Condition !(>node == >state->settings), taking false
>>>> branch.
>>>>
>>>> 1252list_for_each_entry(setting, >state->settings,
>>>> node) {
>>>>
>>>> 5. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
>>>> branch.
>>>> 9. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
>>>> branch.
>>>> 1253if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>>>> 6. Continuing loop.
>>>> 10. Continuing loop.
>>>>
>>>> 1254continue;
>>>> 1255pinmux_disable_setting(setting);
>>>> 1256}
>>>> 1257}
>>>> 1258
>>>> 1259p->state = NULL;
>>>> 1260
>>>> 1261/* Apply all the settings for the new state - pinmux first */
>>>>
>>>> 13. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>> 14. Condition !(>node == >settings), taking true
>>>> branch.
>>>> 1262list_for_each_entry(setting, >settings, node) {
>>>> 15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
>>>>
>>>> 1263switch (setting->type) {
>>>> 1264case PIN_MAP_TYPE_MUX_GROUP:
>>>> 1265ret = pinmux_enable_setting(setting);
>>>> 1266break;
>>>> 1267case PIN_MAP_TYPE_CONFIGS_PIN:
>>>> 1268case PIN_MAP_TYPE_CONFIGS_GROUP:
>>>>
>>>> 16. Breaking from switch.
>>>>
>>>> 1269
Re: pinctrl: core: Handling pinmux and pinconf separately
On 11/03/2021 11:16, Michal Simek wrote:
>
>
> On 3/11/21 11:57 AM, Colin Ian King wrote:
>> Hi,
>>
>> Static analysis on linux-next with Coverity has found a potential issue
>> in drivers/pinctrl/core.c with the following commit:
>>
>> commit 0952b7ec1614abf232e921aac0cc2bca8e60e162
>> Author: Michal Simek
>> Date: Wed Mar 10 09:16:54 2021 +0100
>>
>> pinctrl: core: Handling pinmux and pinconf separately
>>
>> The analysis is as follows:
>>
>> 1234 /**
>> 1235 * pinctrl_commit_state() - select/activate/program a pinctrl state
>> to HW
>> 1236 * @p: the pinctrl handle for the device that requests configuration
>> 1237 * @state: the state handle to select/activate/program
>> 1238 */
>> 1239 static int pinctrl_commit_state(struct pinctrl *p, struct
>> pinctrl_state *state)
>> 1240 {
>> 1241struct pinctrl_setting *setting, *setting2;
>> 1242struct pinctrl_state *old_state = p->state;
>>
>> 1. var_decl: Declaring variable ret without initializer.
>>
>> 1243int ret;
>> 1244
>>
>> 2. Condition p->state, taking true branch.
>>
>> 1245if (p->state) {
>> 1246/*
>> 1247 * For each pinmux setting in the old state, forget
>> SW's record
>> 1248 * of mux owner for that pingroup. Any pingroups
>> which are
>> 1249 * still owned by the new state will be re-acquired
>> by the call
>> 1250 * to pinmux_enable_setting() in the loop below.
>> 1251 */
>>
>> 3. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
>> 4. Condition !(>node == >state->settings), taking true
>> branch.
>> 7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
>> 8. Condition !(>node == >state->settings), taking true
>> branch.
>> 11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
>> 12. Condition !(>node == >state->settings), taking false
>> branch.
>>
>> 1252list_for_each_entry(setting, >state->settings,
>> node) {
>>
>> 5. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
>> branch.
>> 9. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
>> branch.
>> 1253if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>> 6. Continuing loop.
>> 10. Continuing loop.
>>
>> 1254continue;
>> 1255pinmux_disable_setting(setting);
>> 1256}
>> 1257}
>> 1258
>> 1259p->state = NULL;
>> 1260
>> 1261/* Apply all the settings for the new state - pinmux first */
>>
>> 13. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
>> 14. Condition !(>node == >settings), taking true branch.
>> 1262list_for_each_entry(setting, >settings, node) {
>> 15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
>>
>> 1263switch (setting->type) {
>> 1264case PIN_MAP_TYPE_MUX_GROUP:
>> 1265ret = pinmux_enable_setting(setting);
>> 1266break;
>> 1267case PIN_MAP_TYPE_CONFIGS_PIN:
>> 1268case PIN_MAP_TYPE_CONFIGS_GROUP:
>>
>> 16. Breaking from switch.
>>
>> 1269break;
>> 1270default:
>> 1271ret = -EINVAL;
>> 1272break;
>> 1273}
>> 1274
>>
>> Uninitialized scalar variable (UNINIT)
>> 17. uninit_use: Using uninitialized value ret.
>>
>> 1275if (ret < 0)
>> 1276goto unapply_new_state;
>>
>> For the PIN_MAP_TYPE_CONFIGS_PIN and PIN_MAP_TYPE_CONFIGS_GROUP
>> setting->type cases the loop can break out with ret not being set. Since
>> ret has not been initialized it the ret < 0 check is checking against an
>> uninitialized value.
>>
>> I was not sure if the PIN_MAP_TYPE_CONFIGS_PIN and
>> PIN_MAP_TYPE_CONFIGS_GROUP cases should be setting ret and if so, what
>> th
re: scsi: sg: Replace sg_allow_access()
Hi,
Static analysis on linux-next with Coverity has detected an issue in
drivers/scsi/sg.c in function sg_remove_sfp_usercontext with the
following recent commit:
commit 0c32296d73ec5dec64729eb555f1a29ded8a7272
Author: Douglas Gilbert
Date: Fri Feb 19 21:00:28 2021 -0500
scsi: sg: Replace sg_allow_access()
The analysis is as follows:
3913if (unlikely(sfp != e_sfp))
3914SG_LOG(1, sfp, "%s: xa_erase() return unexpected\n",
3915 __func__);
deref_ptr_in_call: Dereferencing pointer sdp.
3916o_count = atomic_dec_return(>open_cnt);
3917SG_LOG(3, sfp, "%s: dev o_count after=%d: sfp=0x%pK --\n",
__func__,
3918 o_count, sfp);
3919kfree(sfp);
3920
Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking sdp suggests that it may be null,
but it has already been dereferenced on all paths leading to the check.
3921if (sdp) {
3922scsi_device_put(sdp->device);
3923kref_put(>d_ref, sg_device_destroy);
3924}
Line 3916 dereferences pointer sdp with >open_cnt, however later on
in line 3921 sdp is being null checked. Either the null check is
redundant if sdp is never null or there is a potential null pointer
dereference on line 3916.
Colin
re: pinctrl: core: Handling pinmux and pinconf separately
Hi,
Static analysis on linux-next with Coverity has found a potential issue
in drivers/pinctrl/core.c with the following commit:
commit 0952b7ec1614abf232e921aac0cc2bca8e60e162
Author: Michal Simek
Date: Wed Mar 10 09:16:54 2021 +0100
pinctrl: core: Handling pinmux and pinconf separately
The analysis is as follows:
1234 /**
1235 * pinctrl_commit_state() - select/activate/program a pinctrl state
to HW
1236 * @p: the pinctrl handle for the device that requests configuration
1237 * @state: the state handle to select/activate/program
1238 */
1239 static int pinctrl_commit_state(struct pinctrl *p, struct
pinctrl_state *state)
1240 {
1241struct pinctrl_setting *setting, *setting2;
1242struct pinctrl_state *old_state = p->state;
1. var_decl: Declaring variable ret without initializer.
1243int ret;
1244
2. Condition p->state, taking true branch.
1245if (p->state) {
1246/*
1247 * For each pinmux setting in the old state, forget
SW's record
1248 * of mux owner for that pingroup. Any pingroups
which are
1249 * still owned by the new state will be re-acquired
by the call
1250 * to pinmux_enable_setting() in the loop below.
1251 */
3. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
4. Condition !(>node == >state->settings), taking true
branch.
7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
8. Condition !(>node == >state->settings), taking true
branch.
11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
12. Condition !(>node == >state->settings), taking false
branch.
1252list_for_each_entry(setting, >state->settings,
node) {
5. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
branch.
9. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
branch.
1253if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
6. Continuing loop.
10. Continuing loop.
1254continue;
1255pinmux_disable_setting(setting);
1256}
1257}
1258
1259p->state = NULL;
1260
1261/* Apply all the settings for the new state - pinmux first */
13. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
14. Condition !(>node == >settings), taking true branch.
1262list_for_each_entry(setting, >settings, node) {
15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
1263switch (setting->type) {
1264case PIN_MAP_TYPE_MUX_GROUP:
1265ret = pinmux_enable_setting(setting);
1266break;
1267case PIN_MAP_TYPE_CONFIGS_PIN:
1268case PIN_MAP_TYPE_CONFIGS_GROUP:
16. Breaking from switch.
1269break;
1270default:
1271ret = -EINVAL;
1272break;
1273}
1274
Uninitialized scalar variable (UNINIT)
17. uninit_use: Using uninitialized value ret.
1275if (ret < 0)
1276goto unapply_new_state;
For the PIN_MAP_TYPE_CONFIGS_PIN and PIN_MAP_TYPE_CONFIGS_GROUP
setting->type cases the loop can break out with ret not being set. Since
ret has not been initialized it the ret < 0 check is checking against an
uninitialized value.
I was not sure if the PIN_MAP_TYPE_CONFIGS_PIN and
PIN_MAP_TYPE_CONFIGS_GROUP cases should be setting ret and if so, what
the value of ret should be set to (is it an error condition or not?). Or
should ret be initialized to 0 or a default error value at the start of
the function.
Hence I'm reporting this issue.
Colin
re: scsi: sg: NO_DXFER move to/from kernel buffers
Hi,
Static analysis on linux-next with Coverity has detected an issue in
drivers/scsi/sg.c with the following recent commit:
commit b32ac463cb59e758b4560260fd168a2b4ea6e81a
Author: Douglas Gilbert
Date: Fri Feb 19 21:00:54 2021 -0500
scsi: sg: NO_DXFER move to/from kernel buffers
The analysis is as follows:
2973 sg_rq_map_kern(struct sg_request *srp, struct request_queue *q,
struct request *rqq, int rw_ind)
2974 {
2975struct sg_scatter_hold *schp = >sgat_h;
2976struct bio *bio;
1. var_decl: Declaring variable k without initializer.
2977int k, ln;
2978int op_flags = 0;
2979int num_sgat = schp->num_sgat;
2980int dlen = schp->dlen;
2981int pg_sz = 1 << (PAGE_SHIFT + schp->page_order);
2982int num_segs = (1 << schp->page_order) * num_sgat;
2983int res = 0;
2984
2. Condition _sdp, taking true branch.
3. Condition _sdp->disk, taking true branch.
4. Condition !!(_sdp && _sdp->disk), taking true branch.
5. Condition !!(((scsi_logging_level >> 3) & 7U /* (1 << 3) - 1 */)
> 4), taking true branch.
6. Condition !!(((scsi_logging_level >> 3) & 7U /* (1 << 3) - 1 */)
> 4), taking true branch.
7. Falling through to end of if statement.
2985SG_LOG(4, srp->parentfp, "%s: dlen=%d, pg_sz=%d\n",
__func__, dlen, pg_sz);
8. Condition num_sgat <= 0, taking false branch.
2986if (num_sgat <= 0)
2987return 0;
9. Condition rw_ind == 1, taking true branch.
2988if (rw_ind == WRITE)
2989op_flags = REQ_SYNC | REQ_IDLE;
Uninitialized scalar variable
10. uninit_use: Using uninitialized value k.
2990bio = sg_mk_kern_bio(num_sgat - k);
2991if (!bio)
Variable k is not initialized, however it is being read when it contains
a garbage value.
Colin
net: mscc: ocelot: issue with uninitialized pointer read in ocelot_flower_parse_key
Hi,
Static analysis with Coverity had detected an uninitialized pointer read
in function ocelot_flower_parse_key in
drivers/net/ethernet/mscc/ocelot_flower.c introduced by commit:
commit 75944fda1dfe836fdd406bef6cb3cc8a80f7af83
Author: Xiaoliang Yang
Date: Fri Oct 2 15:02:23 2020 +0300
net: mscc: ocelot: offload ingress skbedit and vlan actions to VCAP IS1
The analysis is as follows:
531
10. Condition flow_rule_match_key(rule,
FLOW_DISSECTOR_KEY_IPV4_ADDRS), taking true branch.
11. Condition proto == 2048, taking true branch.
532if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV4_ADDRS) &&
533proto == ETH_P_IP) {
12. var_decl: Declaring variable match without initializer.
534struct flow_match_ipv4_addrs match;
535u8 *tmp;
536
13. Condition filter->block_id == VCAP_ES0, taking false branch.
537if (filter->block_id == VCAP_ES0) {
538NL_SET_ERR_MSG_MOD(extack,
539 "VCAP ES0 cannot match on
IP address");
540return -EOPNOTSUPP;
541}
542
14. Condition filter->block_id == VCAP_IS1, taking true branch.
Uninitialized pointer read (UNINIT)
15. uninit_use: Using uninitialized value match.mask.
543if (filter->block_id == VCAP_IS1 && *(u32
*)>dst) {
544NL_SET_ERR_MSG_MOD(extack,
545 "Key type S1_NORMAL cannot
match on destination IP");
546return -EOPNOTSUPP;
547}
match is declared in line 534 and is not initialized and the
uninitialized match.mask is being dereferenced on line 543. Not sure
what intent was on this and how to fix, hence I'm reporting this issue.
Colin
Re: [PATCH] usb: dwc3: Fix dereferencing of null dwc->usb_psy
On 03/03/2021 21:29, Heiko Thiery wrote:
> Hi all,
>
>> On Wed, Mar 3, 2021 at 6:00 PM Colin King wrote:
>>>
>>> From: Colin Ian King
>>>
>>> Currently the null check logic on dwc->usb_psy is inverted as it allows
>>> calls to power_supply_put with a null dwc->usb_psy causing a null
>>> pointer dereference. Fix this by removing the ! operator.
>>>
>>> Addresses-Coverity: ("Dereference after null check")
>>> Fixes: 59fa3def35de ("usb: dwc3: add a power supply for current control")
>>
>> Acked-by: Ray Chi
>>
>>> Signed-off-by: Colin Ian King
>
> Tested-by: Heiko Thiery
Thanks for testing. Much appreciated.
Colin
>
>>> ---
>>> drivers/usb/dwc3/core.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index d15f065849cd..94fdbe502ce9 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1628,7 +1628,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>> assert_reset:
>>> reset_control_assert(dwc->reset);
>>>
>>> - if (!dwc->usb_psy)
>>> + if (dwc->usb_psy)
>>> power_supply_put(dwc->usb_psy);
>>>
>>> return ret;
>>> @@ -1653,7 +1653,7 @@ static int dwc3_remove(struct platform_device *pdev)
>>> dwc3_free_event_buffers(dwc);
>>> dwc3_free_scratch_buffers(dwc);
>>>
>>> - if (!dwc->usb_psy)
>>> + if (dwc->usb_psy)
>>> power_supply_put(dwc->usb_psy);
>>>
>>> return 0;
>>> --
>>> 2.30.0
>>>
>
> Thank you.
>
Re: f2fs_convert_inline_inode causing rebalance based on random uninitialized value in dn.node_changed
On 03/03/2021 19:44, Jaegeuk Kim wrote:
> On 03/02, Colin Ian King wrote:
>> Hi,
>>
>> Static analysis on linux-next detected a potential uninitialized
>> variable dn.node_changed that does not get set when a call to
>> f2fs_get_node_page() fails. This uninitialized value gets used in the
>> call to f2fs_balance_fs() that may or not may not balances dirty node
>> and dentry pages depending on the uninitialized state of the variable.
>>
>> I believe the issue was introduced by commit:
>>
>> commit 2a3407607028f7c780f1c20faa4e922bf631d340
>> Author: Jaegeuk Kim
>> Date: Tue Dec 22 13:23:35 2015 -0800
>>
>> f2fs: call f2fs_balance_fs only when node was changed
>>
>>
>> The analysis is a follows:
>>
>> 184 int f2fs_convert_inline_inode(struct inode *inode)
>> 185 {
>> 186struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>
>>1. var_decl: Declaring variable dn without initializer.
>>
>> 187struct dnode_of_data dn;
>>
>>NOTE dn is not initialized here.
>>
>> 188struct page *ipage, *page;
>> 189int err = 0;
>> 190
>>
>>2. Condition !f2fs_has_inline_data(inode), taking false branch.
>>3. Condition f2fs_hw_is_readonly(sbi), taking false branch.
>>4. Condition f2fs_readonly(sbi->sb), taking false branch.
>>
>> 191if (!f2fs_has_inline_data(inode) ||
>> 192f2fs_hw_is_readonly(sbi) ||
>> f2fs_readonly(sbi->sb))
>> 193return 0;
>> 194
>> 195err = dquot_initialize(inode);
>>
>>5. Condition err, taking false branch.
>>
>> 196if (err)
>> 197return err;
>> 198
>> 199page = f2fs_grab_cache_page(inode->i_mapping, 0, false);
>>
>>6. Condition !page, taking false branch.
>>
>> 200if (!page)
>> 201return -ENOMEM;
>> 202
>> 203f2fs_lock_op(sbi);
>> 204
>> 205ipage = f2fs_get_node_page(sbi, inode->i_ino);
>>
>>7. Condition IS_ERR(ipage), taking true branch.
>>
>> 206if (IS_ERR(ipage)) {
>> 207err = PTR_ERR(ipage);
>>
>>8. Jumping to label out.
>>
>> 208goto out;
>> 209}
>> 210
>>
>>NOTE: set_new_dnode memset's dn so sets the flag to false, but we
>> don't get to this memset if IS_ERR(ipage) above is true.
>>
>> 211set_new_dnode(, inode, ipage, ipage, 0);
>> 212
>> 213if (f2fs_has_inline_data(inode))
>> 214err = f2fs_convert_inline_page(, page);
>> 215
>> 216f2fs_put_dnode();
>> 217 out:
>> 218f2fs_unlock_op(sbi);
>> 219
>> 220f2fs_put_page(page, 1);
>> 221
>>
>> Uninitialized scalar variable:
>>
>>9. uninit_use_in_call: Using uninitialized value dn.node_changed when
>> calling f2fs_balance_fs.
>>
>> 222f2fs_balance_fs(sbi, dn.node_changed);
>> 223
>> 224return err;
>> 225 }
>>
>> I think a suitable fix will be to set dn.node_changed to false on in
>> line 207-208 but I'm concerned if I'm missing something subtle to the
>> rebalancing if I do this.
>>
>> Comments?
>
> Thank you for the report. Yes, it seems that's a right call and we need to
> check the error to decide calling f2fs_balance_fs() in line 222, since
> set_new_dnode() is used to set all the fields in dnode_of_data. So, if you
> don't mind, could you please post a patch?
Just to clarify, just setting dn.node_changes to false is enough?
I'm not entirely sure what you meant when you wrote "and we need to
check the error to decide calling f2fs_balance_fs() in line 222".
Colin
>
> Thanks,
>
>>
>> Colin
>>
re: drm/amd/display: Implement dmub trace event
Hi,
Static analysis on linux-next wit Coverity has found a potential null
pointer dereference in commit:
commit 70732504c53b2d3aae2cebc457515a304672d5bb
Author: Yongqiang Sun
Date: Fri Feb 19 14:50:23 2021 -0500
drm/amd/display: Implement dmub trace event
The analysis is as follows:
400 enum dmub_status dmub_srv_hw_init(struct dmub_srv *dmub,
401 const struct dmub_srv_hw_params
*params)
402 {
403struct dmub_fb *inst_fb = params->fb[DMUB_WINDOW_0_INST_CONST];
404struct dmub_fb *stack_fb = params->fb[DMUB_WINDOW_1_STACK];
405struct dmub_fb *data_fb = params->fb[DMUB_WINDOW_2_BSS_DATA];
406struct dmub_fb *bios_fb = params->fb[DMUB_WINDOW_3_VBIOS];
407struct dmub_fb *mail_fb = params->fb[DMUB_WINDOW_4_MAILBOX];
408struct dmub_fb *tracebuff_fb =
params->fb[DMUB_WINDOW_5_TRACEBUFF];
409struct dmub_fb *fw_state_fb = params->fb[DMUB_WINDOW_6_FW_STATE];
410struct dmub_fb *scratch_mem_fb =
params->fb[DMUB_WINDOW_7_SCRATCH_MEM];
411
412struct dmub_rb_init_params rb_params, outbox0_rb_params;
413struct dmub_window cw0, cw1, cw2, cw3, cw4, cw5, cw6;
414struct dmub_region inbox1, outbox1, outbox0;
415
1. Condition !dmub->sw_init, taking false branch.
416if (!dmub->sw_init)
417return DMUB_STATUS_INVALID;
418
419dmub->fb_base = params->fb_base;
420dmub->fb_offset = params->fb_offset;
421dmub->psp_version = params->psp_version;
422
2. Condition dmub->hw_funcs.reset, taking true branch.
423if (dmub->hw_funcs.reset)
424dmub->hw_funcs.reset(dmub);
425
3. Condition inst_fb, taking true branch.
4. Condition data_fb, taking true branch.
426if (inst_fb && data_fb) {
427cw0.offset.quad_part = inst_fb->gpu_addr;
428cw0.region.base = DMUB_CW0_BASE;
429cw0.region.top = cw0.region.base + inst_fb->size - 1;
430
431cw1.offset.quad_part = stack_fb->gpu_addr;
432cw1.region.base = DMUB_CW1_BASE;
433cw1.region.top = cw1.region.base + stack_fb->size - 1;
434
5. Condition params->load_inst_const, taking true branch.
6. Condition dmub->hw_funcs.backdoor_load, taking true branch.
435if (params->load_inst_const &&
dmub->hw_funcs.backdoor_load) {
436/**
437 * Read back all the instruction memory so we
don't hang the
438 * DMCUB when backdoor loading if the write from
x86 hasn't been
439 * flushed yet. This only occurs in backdoor loading.
440 */
441dmub_flush_buffer_mem(inst_fb);
442dmub->hw_funcs.backdoor_load(dmub, , );
443}
444
445}
446
7. Condition inst_fb, taking true branch.
8. Condition data_fb, taking true branch.
9. Condition bios_fb, taking true branch.
10. Condition mail_fb, taking true branch.
11. Condition tracebuff_fb, taking false branch.
12. var_compare_op: Comparing tracebuff_fb to null implies that
tracebuff_fb might be null.
447if (inst_fb && data_fb && bios_fb && mail_fb && tracebuff_fb &&
448fw_state_fb && scratch_mem_fb) {
449cw2.offset.quad_part = data_fb->gpu_addr;
450cw2.region.base = DMUB_CW0_BASE + inst_fb->size;
451cw2.region.top = cw2.region.base + data_fb->size;
452
453cw3.offset.quad_part = bios_fb->gpu_addr;
454cw3.region.base = DMUB_CW3_BASE;
455cw3.region.top = cw3.region.base + bios_fb->size;
456
457cw4.offset.quad_part = mail_fb->gpu_addr;
458cw4.region.base = DMUB_CW4_BASE;
459cw4.region.top = cw4.region.base + mail_fb->size;
460
461/**
462 * Doubled the mailbox region to accomodate inbox and
outbox.
463 * Note: Currently, currently total mailbox size is
16KB. It is split
464 * equally into 8KB between inbox and outbox. If this
config is
465 * changed, then uncached base address configuration
of outbox1
466 * has to be updated in funcs->setup_out_mailbox.
467 */
468inbox1.base = cw4.region.base;
469inbox1.top = cw4.region.base + DMUB_RB_SIZE;
470outbox1.base = inbox1.top;
471outbox1.top = cw4.region.top;
472
473cw5.offset.quad_part = tracebuff_fb->gpu_addr;
474cw5.region.base = DMUB_CW5_BASE;
475cw5.region.top = cw5.region.base + tracebuff_fb->size;
476
477outbox0.base = DMUB_REGION5_BASE +
TRACE_BUFFER_ENTRY_OFFSET;
478outbox0.top = outbox0.base + sizeof(struct
dmcub_trace_buf_entry) * PERF_TRACE_MAX_ENTRY;
479
480
481cw6.offset.quad_part =
Re: [PATCH][next] mtd: nand: Fix error handling in nand_prog_page_op
On 03/03/2021 09:46, Miquel Raynal wrote:
> Hi Colin,
>
> Colin King wrote on Wed, 3 Mar 2021
> 09:42:46 +:
>
>> From: Colin Ian King
>>
>> The less than zero comparison with status is always false because status
>> is a u8. Fix this by using ret as the return check for the call to
>> chip->legacy.waitfunc() and checking on this and assigning status to ret
>> if it is OK.
>>
>> Addresses-Coverity: ("Unsigned compared against 0")
>> Fixes: 52f67def97f1 ("mtd: nand: fix error handling in nand_prog_page_op()
>> #1")
>> Signed-off-by: Colin Ian King
>
> Thanks for the fix, but this has been handled just an hour ago :)
Ah, missed that. Glad to know it's fixed.
>
> Cheers,
> Miquèl
>
f2fs_convert_inline_inode causing rebalance based on random uninitialized value in dn.node_changed
Hi,
Static analysis on linux-next detected a potential uninitialized
variable dn.node_changed that does not get set when a call to
f2fs_get_node_page() fails. This uninitialized value gets used in the
call to f2fs_balance_fs() that may or not may not balances dirty node
and dentry pages depending on the uninitialized state of the variable.
I believe the issue was introduced by commit:
commit 2a3407607028f7c780f1c20faa4e922bf631d340
Author: Jaegeuk Kim
Date: Tue Dec 22 13:23:35 2015 -0800
f2fs: call f2fs_balance_fs only when node was changed
The analysis is a follows:
184 int f2fs_convert_inline_inode(struct inode *inode)
185 {
186struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
1. var_decl: Declaring variable dn without initializer.
187struct dnode_of_data dn;
NOTE dn is not initialized here.
188struct page *ipage, *page;
189int err = 0;
190
2. Condition !f2fs_has_inline_data(inode), taking false branch.
3. Condition f2fs_hw_is_readonly(sbi), taking false branch.
4. Condition f2fs_readonly(sbi->sb), taking false branch.
191if (!f2fs_has_inline_data(inode) ||
192f2fs_hw_is_readonly(sbi) ||
f2fs_readonly(sbi->sb))
193return 0;
194
195err = dquot_initialize(inode);
5. Condition err, taking false branch.
196if (err)
197return err;
198
199page = f2fs_grab_cache_page(inode->i_mapping, 0, false);
6. Condition !page, taking false branch.
200if (!page)
201return -ENOMEM;
202
203f2fs_lock_op(sbi);
204
205ipage = f2fs_get_node_page(sbi, inode->i_ino);
7. Condition IS_ERR(ipage), taking true branch.
206if (IS_ERR(ipage)) {
207err = PTR_ERR(ipage);
8. Jumping to label out.
208goto out;
209}
210
NOTE: set_new_dnode memset's dn so sets the flag to false, but we
don't get to this memset if IS_ERR(ipage) above is true.
211set_new_dnode(, inode, ipage, ipage, 0);
212
213if (f2fs_has_inline_data(inode))
214err = f2fs_convert_inline_page(, page);
215
216f2fs_put_dnode();
217 out:
218f2fs_unlock_op(sbi);
219
220f2fs_put_page(page, 1);
221
Uninitialized scalar variable:
9. uninit_use_in_call: Using uninitialized value dn.node_changed when
calling f2fs_balance_fs.
222f2fs_balance_fs(sbi, dn.node_changed);
223
224return err;
225 }
I think a suitable fix will be to set dn.node_changed to false on in
line 207-208 but I'm concerned if I'm missing something subtle to the
rebalancing if I do this.
Comments?
Colin
Re: [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[]
On 02/03/2021 08:44, Krzysztof Kozlowski wrote:
> On Tue, Feb 23, 2021 at 07:38:21PM +, Colin King wrote:
>> From: Colin Ian King
>>
>> Currently the array gpmc_cs is indexed by cs before it cs is range checked
>> and the pointer read from this out-of-index read is dereferenced. Fix this
>> by performing the range check on cs before the read and the following
>> pointer dereference.
>>
>> Addresses-Coverity: ("Negative array index read")
>> Fixes: 186401937927 ("memory: gpmc: Move omap gpmc code to live under
>> drivers")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/memory/omap-gpmc.c | 7 +--
>
> Thanks, applied with Tony's ack and changed Fixes to 9ed7a776eb50 ("ARM:
> OMAP2+: Fix support for multiple devices on a GPMC chip select").
Thanks for correcting the Fixes line
>
> Best regards,
> Krzysztof
>
re: cifs: Retain old ACEs when converting between mode bits and ACL.
Hi,
Static analysis on linux-next with Coverity had detected a potential
null pointer dereference with the following commit:
commit f5065508897a922327f32223082325d10b069ebc
Author: Shyam Prasad N
Date: Fri Feb 12 04:38:43 2021 -0800
cifs: Retain old ACEs when converting between mode bits and ACL.
The analysis is as follows:
1258 /* Convert permission bits from mode to equivalent CIFS ACL */
1259 static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd
*pnntsd,
1260__u32 secdesclen, __u32 *pnsecdesclen, __u64 *pnmode, kuid_t
uid, kgid_t gid,
1261bool mode_from_sid, bool id_from_sid, int *aclflag)
1262 {
1263int rc = 0;
1264__u32 dacloffset;
1265__u32 ndacloffset;
1266__u32 sidsoffset;
1267struct cifs_sid *owner_sid_ptr, *group_sid_ptr;
1268struct cifs_sid *nowner_sid_ptr = NULL, *ngroup_sid_ptr = NULL;
1. assign_zero: Assigning: dacl_ptr = NULL.
1269struct cifs_acl *dacl_ptr = NULL; /* no need for SACL ptr */
1270struct cifs_acl *ndacl_ptr = NULL; /* no need for SACL ptr */
1271char *end_of_acl = ((char *)pntsd) + secdesclen;
1272u16 size = 0;
1273
1274dacloffset = le32_to_cpu(pntsd->dacloffset);
2. Condition dacloffset, taking false branch.
1275if (dacloffset) {
1276dacl_ptr = (struct cifs_acl *)((char *)pntsd +
dacloffset);
1277if (end_of_acl < (char *)dacl_ptr +
le16_to_cpu(dacl_ptr->size)) {
1278cifs_dbg(VFS, "Existing ACL size is wrong.
Discarding old ACL\n");
1279dacl_ptr = NULL;
NOTE: dacl_ptr is set to NULL and dacloffset is true
1280}
1281}
1282
1283owner_sid_ptr = (struct cifs_sid *)((char *)pntsd +
1284le32_to_cpu(pntsd->osidoffset));
1285group_sid_ptr = (struct cifs_sid *)((char *)pntsd +
1286le32_to_cpu(pntsd->gsidoffset));
1287
3. Condition pnmode, taking true branch.
4. Condition *pnmode != 18446744073709551615ULL, taking false
branch.
1288if (pnmode && *pnmode != NO_CHANGE_64) { /* chmod */
1289ndacloffset = sizeof(struct cifs_ntsd);
1290ndacl_ptr = (struct cifs_acl *)((char *)pnntsd +
ndacloffset);
1291ndacl_ptr->revision =
1292dacloffset ? dacl_ptr->revision :
cpu_to_le16(ACL_REVISION);
1293
1294ndacl_ptr->size = cpu_to_le16(0);
1295ndacl_ptr->num_aces = cpu_to_le32(0);
1296
1297rc = set_chmod_dacl(dacl_ptr, ndacl_ptr,
owner_sid_ptr, group_sid_ptr,
1298pnmode, mode_from_sid);
1299
1300sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
1301/* copy the non-dacl portion of secdesc */
1302*pnsecdesclen = copy_sec_desc(pntsd, pnntsd, sidsoffset,
1303NULL, NULL);
1304
1305*aclflag |= CIFS_ACL_DACL;
1306} else {
1307ndacloffset = sizeof(struct cifs_ntsd);
1308ndacl_ptr = (struct cifs_acl *)((char *)pnntsd +
ndacloffset);
5. Condition dacloffset, taking false branch.
1309ndacl_ptr->revision =
1310dacloffset ? dacl_ptr->revision :
cpu_to_le16(ACL_REVISION);
Explicit null dereferenced (FORWARD_NULL)
6. var_deref_op: Dereferencing null pointer dacl_ptr.
1311ndacl_ptr->num_aces = dacl_ptr->num_aces;
Line 1309..1311, when dacloffset and dacl_ptr is null we hit a null ptr
dereference on dacl_ptr.
Re: [PATCH] memory: gpmc: fix out of bounds read and dereference on gpmc_cs[]
On 24/02/2021 07:55, Dan Carpenter wrote:
> On Tue, Feb 23, 2021 at 07:38:21PM +, Colin King wrote:
>> From: Colin Ian King
>>
>> Currently the array gpmc_cs is indexed by cs before it cs is range checked
>> and the pointer read from this out-of-index read is dereferenced. Fix this
>> by performing the range check on cs before the read and the following
>> pointer dereference.
>>
>> Addresses-Coverity: ("Negative array index read")
>> Fixes: 186401937927 ("memory: gpmc: Move omap gpmc code to live under
>> drivers")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/memory/omap-gpmc.c | 7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
>> index cfa730cfd145..f80c2ea39ca4 100644
>> --- a/drivers/memory/omap-gpmc.c
>> +++ b/drivers/memory/omap-gpmc.c
>> @@ -1009,8 +1009,8 @@ EXPORT_SYMBOL(gpmc_cs_request);
>>
>> void gpmc_cs_free(int cs)
>> {
>> -struct gpmc_cs_data *gpmc = _cs[cs];
>> -struct resource *res = >mem;
> > There is no actual dereferencing going on here, it just taking the
> addresses. But the patch is also harmless and improves readability.
Plus compilers are getting smarter with static analysis so some day in
the future they will warn about this.
>
> regards,
> dan carpenter
>
NACK: [PATCH][next] tracing/tools: fix spelling mistake "functionph" -> "graph"
On 16/02/2021 14:01, Colin King wrote: > From: Colin Ian King > > There is a spelling mistake in the -g help option, I believe > it should be "graph". Fix it. > > Signed-off-by: Colin Ian King > --- > tools/tracing/latency/latency-collector.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/tracing/latency/latency-collector.c > b/tools/tracing/latency/latency-collector.c > index 57b20802e71b..8d28234cd6fb 100644 > --- a/tools/tracing/latency/latency-collector.c > +++ b/tools/tracing/latency/latency-collector.c > @@ -1711,7 +1711,7 @@ static void show_usage(void) > "\t\t\tbeginning, end, and backtrace.\n\n" > > "-g, --graph\t\tEnable the display-graph option in trace_option. This\n" > -"\t\t\toption causes ftrace to show the functionph of how\n" > +"\t\t\toption causes ftrace to show the graph of how\n" > "\t\t\tfunctions are calling other functions.\n\n" > > "-c, --policy POL\tRun the program with scheduling policy POL. POL can be\n" > Found another spelling mistake, sending a V2 soon.
re: octeontx2-af: cn10k: MAC internal loopback support
Hi,
Static analysis on linux-next today using Coverity found an issue in the
following commit:
commit 3ad3f8f93c81f81d6e28b2e286b03669cc1fb3b0
Author: Hariprasad Kelam
Date: Thu Feb 11 21:28:34 2021 +0530
octeontx2-af: cn10k: MAC internal loopback support
The analysis is as follows:
723 static int rvu_cgx_config_intlbk(struct rvu *rvu, u16 pcifunc, bool en)
724 {
725struct mac_ops *mac_ops;
1. var_decl: Declaring variable lmac_id without initializer.
726u8 cgx_id, lmac_id;
727
2. Condition !is_cgx_config_permitted(rvu, pcifunc), taking false branch.
728if (!is_cgx_config_permitted(rvu, pcifunc))
729return -EPERM;
730
Uninitialized scalar variable (UNINIT)
731mac_ops = get_mac_ops(rvu_cgx_pdata(cgx_id, rvu));
732
Uninitialized scalar variable (UNINIT)
3. uninit_use_in_call: Using uninitialized value lmac_id when calling
*mac_ops->mac_lmac_intl_lbk.
733return mac_ops->mac_lmac_intl_lbk(rvu_cgx_pdata(cgx_id, rvu),
734 lmac_id, en);
735 }
Variables cgx_id and lmac_id are no longer being initialized and garbage
values are being passed into function calls. Originally, these
variables were being initialized with a call to rvu_get_cgx_lmac_id()
but that has now been removed.
Colin
Re: [PATCH][next] media: i2c: imx334: Fix a read of the uninitialized variable ret
On 11/02/2021 10:41, Dan Carpenter wrote:
> On Wed, Feb 10, 2021 at 11:03:03PM +0200, Sakari Ailus wrote:
>> Hi Colin,
>>
>> On Wed, Feb 10, 2021 at 07:07:52PM +, Colin King wrote:
>>> From: Colin Ian King
>>>
>>> Currently there is a dev_err error message that is printing the
>>> error status in variable ret (that has not been set) instead of
>>> the correct error status from imx334->reset_gpio. Fix this.
>>>
>>> Addresses-Coverity: ("Uninitialized scalar variable")
>>> Fixes: 9746b11715c3 ("media: i2c: Add imx334 camera sensor driver")
>>>
>>> Signed-off-by: Colin Ian King
>>> ---
>>> drivers/media/i2c/imx334.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
>>> index 07e31bc2ef18..f8b1caf26c9b 100644
>>> --- a/drivers/media/i2c/imx334.c
>>> +++ b/drivers/media/i2c/imx334.c
>>> @@ -790,7 +790,8 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
>>> imx334->reset_gpio = devm_gpiod_get_optional(imx334->dev, "reset",
>>> GPIOD_OUT_LOW);
>>> if (IS_ERR(imx334->reset_gpio)) {
>>> - dev_err(imx334->dev, "failed to get reset gpio %d", ret);
>>> + dev_err(imx334->dev, "failed to get reset gpio %ld",
>>> + IS_ERR_VALUE(imx334->reset_gpio));
>
> IS_ERR_VALUE() isn't right. It would always print 1 here. It should
> just be PTR_ERR().
>
> IS_ERR_VALUE() is like IS_ERR() but for when you're storing memory
> addresses in an unsigned long variable. get_unmapped_area(), for
> example, returns unsigned longs.
>
> regards,
> dan carpenter
>
Thanks, that was a brown paper bug mistake for sure :-/
Colin
Re: [PATCH][next] soc: xilinx: vcu: remove deadcode on null divider check
On 11/02/2021 07:31, Michael Tretter wrote:
> On Wed, 10 Feb 2021 18:49:38 +, Colin King wrote:
>> From: Colin Ian King
>>
>> The pointer 'divider' has previously been null checked followed by
>> a return, hence the subsequent null check is redundant deadcode
>> that can be removed. Clean up the code and remove it.
>>
>> Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output
>> clocks")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/clk/xilinx/xlnx_vcu.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
>> index d66b1315114e..607936d7a413 100644
>> --- a/drivers/clk/xilinx/xlnx_vcu.c
>> +++ b/drivers/clk/xilinx/xlnx_vcu.c
>> @@ -512,9 +512,6 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw
>> *hw)
>>
>> mux = clk_hw_get_parent(divider);
>> clk_hw_unregister_mux(mux);
>> -if (!divider)
>> -return;
>> -
>> clk_hw_unregister_divider(divider);
>
> Thanks for pointing this out. There is actually a different bug there.
>
> There should have been a check for !mux before unregistering the mux:
>
> mux = clk_hw_get_parent(divider);
> clk_hw_unregister_divider(divider);
> if (!mux)
> return;
Ah, that makes sense, I'll send a V2.
>
> clk_hw_unregister_mux(mux);
>
> Michael
>
>> }
>>
>> --
>> 2.30.0
>>
>>
NAK: [PATCH][next] media: uvcvideo: remove duplicated dma_dev assignment
On 10/02/2021 17:45, Colin King wrote:
> From: Colin Ian King
>
> The assignment to dma_dev has been performed twice in one
> statement. Fix this by removing the extraneous assignment.
>
> Addresses-Coverity: ("Evaluation order violation")
> Fixes: fdcd02a641e2 ("media: uvcvideo: Use dma_alloc_noncontiguos API")
> Signed-off-by: Colin Ian King
> ---
> drivers/media/usb/uvc/uvc_video.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c
> index dc81f9a86eca..edf451a956d8 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1105,7 +1105,7 @@ static inline struct device *stream_to_dmadev(struct
> uvc_streaming *stream)
>
> static void uvc_urb_dma_sync(struct uvc_urb *uvc_urb, bool for_device)
> {
> - struct device *dma_dev = dma_dev = stream_to_dmadev(uvc_urb->stream);
> + struct device *dma_dev = stream_to_dmadev(uvc_urb->stream);
>
> if (for_device) {
> dma_sync_sgtable_for_device(dma_dev, uvc_urb->sgt,
>
there are some other occurrences of this, I'll send a V2.
re: mt76: mt7921: add MCU support
Hi,
Static analysis with Coverity on linux-next has found an issue with the
following commit:
commit 1c099ab44727c8e42fe4de4d91b53cec3ef02860
Author: Sean Wang
Date: Thu Jan 28 03:33:39 2021 +0800
mt76: mt7921: add MCU support
The analysis is as follows:
390 static void
391 mt7921_mcu_tx_rate_report(struct mt7921_dev *dev, struct sk_buff *skb,
392 u16 wlan_idx)
393 {
394struct mt7921_mcu_wlan_info_event *wtbl_info =
395(struct mt7921_mcu_wlan_info_event *)(skb->data);
396struct rate_info rate = {};
397u8 curr_idx = wtbl_info->rate_info.rate_idx;
398u16 curr = le16_to_cpu(wtbl_info->rate_info.rate[curr_idx]);
399struct mt7921_mcu_peer_cap peer = wtbl_info->peer_cap;
400struct mt76_phy *mphy = >mphy;
1. var_decl: Declaring variable stats without initializer.
401struct mt7921_sta_stats *stats;
402struct mt7921_sta *msta;
403struct mt76_wcid *wcid;
404
2. Condition wlan_idx >= 288, taking false branch.
405if (wlan_idx >= MT76_N_WCIDS)
406return;
3. Condition 0 /* !sizeof ((*dev).mt76.wcid[wlan_idx]) == sizeof
(char) || sizeof ((*dev).mt76.wcid[wlan_idx]) == sizeof (short)) ||
sizeof ((*dev).mt76.wcid[wlan_idx]) == sizeof (int)) || sizeof
((*dev).mt76.wcid[wlan_idx]) == sizeof (long)) || sizeof
((*dev).mt76.wcid[wlan_idx]) == sizeof (long long)) */, taking false branch.
4. Condition debug_lockdep_rcu_enabled(), taking true branch.
5. Condition !__warned, taking true branch.
6. Condition 0, taking false branch.
7. Condition rcu_read_lock_held(), taking false branch.
407wcid = rcu_dereference(dev->mt76.wcid[wlan_idx]);
8. Condition !wcid, taking true branch.
408if (!wcid) {
Uninitialized pointer write (UNINIT)
9. uninit_use: Using uninitialized value stats.
409stats->tx_rate = rate;
410return;
411}
Line 409 dereferences pointer stats, however, this pointer has not yet
been initialized. The initialization occurs later:
413msta = container_of(wcid, struct mt7921_sta, wcid);
414stats = >stats;
Colin
Re: [PATCH] HID: logitech-dj: fix unintentional integer overflow on left shift
On 08/02/2021 15:06, Dan Carpenter wrote:
> On Sun, Feb 07, 2021 at 11:21:20PM +, Colin King wrote:
>> From: Colin Ian King
>>
>> Shifting the integer value 1 is evaluated using 32-bit rithmetic
>> and then used in an expression that expects a 64-bit value, so
>> there is potentially an integer overflow. Fix this by using th
>> BIT_ULL macro to perform the shift and avoid the overflow.
>>
>> Addresses-Coverity: ("Uninitentional integer overflow")
>> Fixes: 534a7b8e10ec ("HID: Add full support for Logitech Unifying receivers")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/hid/hid-logitech-dj.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>> index 45e7e0bdd382..747f41be0603 100644
>> --- a/drivers/hid/hid-logitech-dj.c
>> +++ b/drivers/hid/hid-logitech-dj.c
>> @@ -1035,7 +1035,7 @@ static void logi_dj_recv_forward_null_report(struct
>> dj_receiver_dev *djrcv_dev,
>> memset(reportbuffer, 0, sizeof(reportbuffer));
>>
>> for (i = 0; i < NUMBER_OF_HID_REPORTS; i++) {
> ^
> This is 32, so it can't be undefined.
Urgh, looks like coverity is being overly pedantic here. :-(
>
>> -if (djdev->reports_supported & (1 << i)) {
>> +if (djdev->reports_supported & BIT_ULL(i)) {
>> reportbuffer[0] = i;
>> if (hid_input_report(djdev->hdev,
>> HID_INPUT_REPORT,
>
> regards,
> dan carpenter
>
Re: [PATCH][next] usb: cdnsp: Fix spelling mistake "delagete" -> "delegate"
On 04/02/2021 09:25, Pawel Laszczak wrote:
>
> I've sent the patch that remove this one and others similar printk from
> driver.
Thanks Pawel.
Colin
>
>>
>>
>> On Thu, Feb 04, 2021 at 05:07:16AM +, Pawel Laszczak wrote:
>>> Hi Dan,
>>>
>>>>> From: Colin Ian King
>>>>>
>>>>> There is a spelling mistake in a literal string. Fix it.
>>>>>
>>>>> Signed-off-by: Colin Ian King
>>>>> ---
>>>>> drivers/usb/cdns3/cdnsp-ep0.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/usb/cdns3/cdnsp-ep0.c b/drivers/usb/cdns3/cdnsp-ep0.c
>>>>> index e2b1bcb3f80e..e30931ebc870 100644
>>>>> --- a/drivers/usb/cdns3/cdnsp-ep0.c
>>>>> +++ b/drivers/usb/cdns3/cdnsp-ep0.c
>>>>> @@ -45,7 +45,7 @@ static int cdnsp_ep0_delegate_req(struct cdnsp_device
>>>>> *pdev,
>>>>> {
>>>>> int ret;
>>>>>
>>>>> - trace_cdnsp_ep0_request("delagete");
>>>>> + trace_cdnsp_ep0_request("delegate");
>>>>>
>>>>
>>>> This printk is useless and should just be deleted. Use ftrace instead.
>>>
>>> Maybe this printk is redundant but it's more comfortable in use.
>>> To debug I can simply enable cdns-dev events (echo cdnsp-dev:* > set_event)
>>> and I will get the full picture of what the driver is doing.
>>>
>>> Otherwise, I must remember which function I need to add to
>>> set_ftrace_filter.
>>> Of course, by default I can simply add all cdnsp* functions (echo cdnsp* >
>>> set_ftrace_filter) but it
>>> increases the trace log and makes it a little more difficult to analyze.
>>>
>>> So maybe in some cases we shouldn't complain for such printk ?
>>>
>>> It's my private opinion and not necessarily correct :)
>>
>> Please don't have duplicate tracepoints for something like "this
>> function is now called", it's redundant.
>>
>
> Thanks,
> Pawel Laszczak
>
Re: [PATCH][next] Input: iqs5xx: Ensure error_bl is initialized on error exit path
On 28/01/2021 14:39, Jeff LaBundy wrote:
> Hi Colin,
>
> On Thu, Jan 28, 2021 at 12:19:03PM +, Colin King wrote:
>> From: Colin Ian King
>>
>> Currently if the call to qs5xx_fw_file_parse fails the error return
>> exit path will read the uninitialized variable error_bl. Fix this
>> by ensuring error_bl is initialized to zero.
>>
>> Addresses-Coverity: ("Uninitialized scalar variable")
>> Fixes: 2539da6677b6 ("Input: iqs5xx - preserve bootloader errors")
>> Signed-off-by: Colin Ian King
>
> This was fixed in [1]; it just needs pushed.
OK, thanks for letting me know.
>
> [1] https://patchwork.kernel.org/patch/12043701
>
>> ---
>> drivers/input/touchscreen/iqs5xx.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/touchscreen/iqs5xx.c
>> b/drivers/input/touchscreen/iqs5xx.c
>> index 05e0c6ff217b..54f30038dca4 100644
>> --- a/drivers/input/touchscreen/iqs5xx.c
>> +++ b/drivers/input/touchscreen/iqs5xx.c
>> @@ -852,7 +852,7 @@ static int iqs5xx_fw_file_parse(struct i2c_client
>> *client,
>> static int iqs5xx_fw_file_write(struct i2c_client *client, const char
>> *fw_file)
>> {
>> struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
>> -int error, error_bl;
>> +int error, error_bl = 0;
>> u8 *pmap;
>>
>> if (iqs5xx->bl_status == IQS5XX_BL_STATUS_NONE)
>> --
>> 2.29.2
>>
>
> Kind regards,
> Jeff LaBundy
>
re: GTP: add support for flow based tunneling API
Hi,
Static analysis of today's linux-next using Coverity has found a
potential memory leak issue in the following commit:
commit 9ab7e76aefc97a9aa664accb59d6e8dc5e52514a
Author: Pravin B Shelar
Date: Sat Jan 9 23:00:21 2021 -0800
GTP: add support for flow based tunneling API
The analysis is as follows:
186 static int gtp_set_tun_dst(struct gtp_dev *gtp, struct sk_buff *skb,
187 unsigned int hdrlen, u8 gtp_version,
188 __be64 tid, u8 flags)
189 {
190struct metadata_dst *tun_dst;
191int opts_len = 0;
192
1. Condition !!(flags & 7), taking true branch.
2. Condition !!(flags & 7), taking true branch.
193if (unlikely(flags & GTP1_F_MASK))
194opts_len = sizeof(struct gtpu_metadata);
195
3. alloc_fn: Storage is returned from allocation function udp_tun_rx_dst.
4. var_assign: Assigning: tun_dst = storage returned from
udp_tun_rx_dst(skb, gtp->sk1u->__sk_common.skc_family, 1024, tid, opts_len).
196tun_dst = udp_tun_rx_dst(skb, gtp->sk1u->sk_family,
TUNNEL_KEY, tid, opts_len);
5. Condition !tun_dst, taking false branch.
197if (!tun_dst) {
198netdev_dbg(gtp->dev, "Failed to allocate tun_dst");
199goto err;
200}
201
6. Condition 0 /* __builtin_types_compatible_p() */, taking false branch.
7. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
8. Falling through to end of if statement.
9. Condition !!branch, taking false branch.
10. Condition ({...; !!branch;}), taking false branch.
202netdev_dbg(gtp->dev, "attaching metadata_dst to skb, gtp ver
%d hdrlen %d\n",
203 gtp_version, hdrlen);
11. Condition !!opts_len, taking true branch.
12. Condition !!opts_len, taking true branch.
204if (unlikely(opts_len)) {
205struct gtpu_metadata *opts;
206struct gtp1_header *gtp1;
207
208opts = ip_tunnel_info_opts(_dst->u.tun_info);
209gtp1 = (struct gtp1_header *)(skb->data +
sizeof(struct udphdr));
210opts->ver = GTP_METADATA_V1;
211opts->flags = gtp1->flags;
212opts->type = gtp1->type;
13. Condition 0 /* __builtin_types_compatible_p() */, taking false
branch.
14. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
15. Falling through to end of if statement.
16. Condition !!branch, taking false branch.
17. Condition ({...; !!branch;}), taking false branch.
213netdev_dbg(gtp->dev, "recved control pkt: flag %x
type: %d\n",
214 opts->flags, opts->type);
215tun_dst->u.tun_info.key.tun_flags |= TUNNEL_GTPU_OPT;
216tun_dst->u.tun_info.options_len = opts_len;
217skb->protocol = htons(0x); /* Unknown */
218}
219/* Get rid of the GTP + UDP headers. */
18. Condition !net_eq(sock_net(gtp->sk1u), dev_net(gtp->dev)), taking
false branch.
19. Condition !net_eq(sock_net(gtp->sk1u), dev_net(gtp->dev)), taking
false branch.
20. Condition iptunnel_pull_header(skb, hdrlen, skb->protocol,
!net_eq(sock_net(gtp->sk1u), dev_net(gtp->dev))), taking true branch.
220if (iptunnel_pull_header(skb, hdrlen, skb->protocol,
221 !net_eq(sock_net(gtp->sk1u),
dev_net(gtp->dev {
222gtp->dev->stats.rx_length_errors++;
21. Jumping to label err.
223goto err;
224}
225
226skb_dst_set(skb, _dst->dst);
227return 0;
228 err:
Resource leak (RESOURCE_LEAK)
22. leaked_storage: Variable tun_dst going out of scope leaks the
storage it points to.
229return -1;
230 }
The goto on line 223 is leaking tun_dst. From what I can see, I believe
a call to kfree(tun_dst) before the goto on line 223 looks like a
pertinent fix, but I'm not 100% sure, so I'm flagging this up as an
issue that need further investigation by folk who are more familiar with
this code.
Colin
Re: [PATCH][next] drm/amdgpu: Add missing BOOTUP_DEFAULT to profile_name[]
On 15/01/2021 10:07, Christophe JAILLET wrote:
> Le 15/01/2021 à 10:37, Colin Ian King a écrit :
>> On 12/01/2021 10:07, Dan Carpenter wrote:
>>> On Mon, Jan 11, 2021 at 11:46:38AM +, Colin King wrote:
>>>> From: Colin Ian King
>>>>
>>>> A recent change added a new BOOTUP_DEFAULT power profile mode
>>>> to the PP_SMC_POWER_PROFILE enum but omitted updating the
>>>> corresponding profile_name array. Fix this by adding in the
>>>> missing BOOTUP_DEFAULT to profile_name[].
>>>>
>>>
>>> Still not enough to prevent the array overflow. It needs POWERSAVE as
>>> well.
>>
>> Thanks for checking, but there is a 1-to-1 relation ship now:
>>
>> enum PP_SMC_POWER_PROFILE {
>> PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT = 0x0,
>> PP_SMC_POWER_PROFILE_FULLSCREEN3D = 0x1,
>> PP_SMC_POWER_PROFILE_POWERSAVING = 0x2,
>> PP_SMC_POWER_PROFILE_VIDEO = 0x3,
>> PP_SMC_POWER_PROFILE_VR = 0x4,
>> PP_SMC_POWER_PROFILE_COMPUTE = 0x5,
>> PP_SMC_POWER_PROFILE_CUSTOM = 0x6,
>> PP_SMC_POWER_PROFILE_COUNT,
>> };
>>
>> vs
>>
>> static const char *profile_name[] = {
>> "BOOTUP_DEFAULT",
>> "3D_FULL_SCREEN",
>> "POWER_SAVING",
>
> This line has been added yesterday in commit f727ebeb589d.
> So Dan was right when he sent his patch, but some else fixed it.
Ah, my bad for not seeing that. :-/
>
> CJ
>
>> "VIDEO",
>> "VR",
>> "COMPUTE",
>> "CUSTOM"};
>>
>>
>> unless I'm missing something because I've not had enough coffee.
>>
>> Colin
>>
>>>
>>> regards,
>>> dan carpenter
>>>
>>
>>
>
Re: [PATCH][next] drm/amdgpu: Add missing BOOTUP_DEFAULT to profile_name[]
On 12/01/2021 10:07, Dan Carpenter wrote:
> On Mon, Jan 11, 2021 at 11:46:38AM +, Colin King wrote:
>> From: Colin Ian King
>>
>> A recent change added a new BOOTUP_DEFAULT power profile mode
>> to the PP_SMC_POWER_PROFILE enum but omitted updating the
>> corresponding profile_name array. Fix this by adding in the
>> missing BOOTUP_DEFAULT to profile_name[].
>>
>
> Still not enough to prevent the array overflow. It needs POWERSAVE as
> well.
Thanks for checking, but there is a 1-to-1 relation ship now:
enum PP_SMC_POWER_PROFILE {
PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT = 0x0,
PP_SMC_POWER_PROFILE_FULLSCREEN3D = 0x1,
PP_SMC_POWER_PROFILE_POWERSAVING = 0x2,
PP_SMC_POWER_PROFILE_VIDEO= 0x3,
PP_SMC_POWER_PROFILE_VR = 0x4,
PP_SMC_POWER_PROFILE_COMPUTE = 0x5,
PP_SMC_POWER_PROFILE_CUSTOM = 0x6,
PP_SMC_POWER_PROFILE_COUNT,
};
vs
static const char *profile_name[] = {
"BOOTUP_DEFAULT",
"3D_FULL_SCREEN",
"POWER_SAVING",
"VIDEO",
"VR",
"COMPUTE",
"CUSTOM"};
unless I'm missing something because I've not had enough coffee.
Colin
>
> regards,
> dan carpenter
>
Re: [PATCH][next] ASoC: soc-pcm: Fix uninitialised return value in variable ret
On 12/01/2021 10:22, Dan Carpenter wrote: > On Mon, Jan 11, 2021 at 05:37:36PM +0000, Colin Ian King wrote: >> On 11/01/2021 16:35, Mark Brown wrote: >>> On Fri, Jan 08, 2021 at 12:35:46PM +, Colin King wrote: >>>> From: Colin Ian King >>>> >>>> Currently when attempting to start the BE fails because the >>>> FE is not started the error return variable ret is not initialized >>>> and garbage is returned. Fix this by setting it to 0 so the >>> >>> This doesn't apply against current code, please check and resend. >>> >> >> Current ASoC tree now has two commits: >> >> commit 4eeed5f40354735c4e68e71904db528ed19c9cbb >> Author: Souptick Joarder >> Date: Sat Jan 9 09:15:01 2021 +0530 >> >> ASoC: soc-pcm: return correct -ERRNO in failure path >> >> commit e91b65b36fde0690f1c694f17dd1b549295464a7 >> Author: Dan Carpenter >> Date: Mon Jan 11 12:50:21 2021 +0300 >> >> ASoC: soc-pcm: Fix an uninitialized error code >> >> ..both set ret to non-zero, which I believe will throw a subsequent >> warning messagethat's not strictly related. > > My patch restored the original behavior. And I think that errors should > return error codes. What you're saying is basically "Returning an error > is a bug because it will trigger an error message in the caller". So > then we have to have a debate about printks as a layering violation. > > I don't like error messages generally, because I think they make the > code messy. A lot of people put error messages for impossible things. > Or if a kmalloc() fails or whatever. There are too many error messages > which people add in an auto-pilot way without considering whether it's > necessary. > > But some people think, and maybe they're correct, that it's best if > every function in the call tree prints a message. That way you can > trace the error path easily. +1 Yep, good point, ignore my fix. Thanks Dan for your observations. > > regards, > dan carpenter >
Re: [PATCH][next] ASoC: soc-pcm: Fix uninitialised return value in variable ret
On 11/01/2021 16:35, Mark Brown wrote: > On Fri, Jan 08, 2021 at 12:35:46PM +, Colin King wrote: >> From: Colin Ian King >> >> Currently when attempting to start the BE fails because the >> FE is not started the error return variable ret is not initialized >> and garbage is returned. Fix this by setting it to 0 so the > > This doesn't apply against current code, please check and resend. > Current ASoC tree now has two commits: commit 4eeed5f40354735c4e68e71904db528ed19c9cbb Author: Souptick Joarder Date: Sat Jan 9 09:15:01 2021 +0530 ASoC: soc-pcm: return correct -ERRNO in failure path commit e91b65b36fde0690f1c694f17dd1b549295464a7 Author: Dan Carpenter Date: Mon Jan 11 12:50:21 2021 +0300 ASoC: soc-pcm: Fix an uninitialized error code ..both set ret to non-zero, which I believe will throw a subsequent warning messagethat's not strictly related. my fix was acked by zhucan...@vivo.com, so I'm now confused what is the *correct* fix. Colin
Re: [PATCH][next] ASoC: soc-pcm: Fix uninitialised return value in variable ret
On 11/01/2021 16:35, Mark Brown wrote: > On Fri, Jan 08, 2021 at 12:35:46PM +, Colin King wrote: >> From: Colin Ian King >> >> Currently when attempting to start the BE fails because the >> FE is not started the error return variable ret is not initialized >> and garbage is returned. Fix this by setting it to 0 so the > > This doesn't apply against current code, please check and resend. > Just to double-check, which tree should I be working against? Colin
Re: platform/surface: Add Surface Aggregator user-space interface (static analysis issues)
On 11/01/2021 14:37, Maximilian Luz wrote:
> On 1/11/21 3:11 PM, Colin Ian King wrote:
>> On 11/01/2021 13:55, Maximilian Luz wrote:
>>> On 1/11/21 1:12 PM, Colin Ian King wrote:
>>>> Hi Maximilian,
>>>>
>>>> Static analysis of linux-next with Coverity has found several issues
>>>> with the following commit:
>>>>
>>>> commit 178f6ab77e617c984d6520b92e747075a12676ff
>>>> Author: Maximilian Luz
>>>> Date: Mon Dec 21 19:39:58 2020 +0100
>>>>
>>>> platform/surface: Add Surface Aggregator user-space interface
>>>>
>>>> The analysis is as follows:
>>>>
>>>> 65static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long
>>>> arg)
>>>> 66{
>>>> 67 struct ssam_cdev_request __user *r;
>>>> 68 struct ssam_cdev_request rqst;
>>>>
>>>> 1. var_decl: Declaring variable spec without initializer.
>>>>
>>>> 69 struct ssam_request spec;
>>>> 70 struct ssam_response rsp;
>>>> 71 const void __user *plddata;
>>>> 72 void __user *rspdata;
>>>> 73 int status = 0, ret = 0, tmp;
>>>> 74
>>>> 75 r = (struct ssam_cdev_request __user *)arg;
>>>> 76 ret = copy_struct_from_user(, sizeof(rqst), r,
>>>> sizeof(*r));
>>>>
>>>> 2. Condition ret, taking true branch.
>>>>
>>>> 77 if (ret)
>>>>
>>>> 3. Jumping to label out.
>>>>
>>>> 78 goto out;
>>>>
>>>> 79
>>>> 80 plddata = u64_to_user_ptr(rqst.payload.data);
>>>> 81 rspdata = u64_to_user_ptr(rqst.response.data);
>>>> 82
>>>> 83 /* Setup basic request fields. */
>>>> 84 spec.target_category = rqst.target_category;
>>>> 85 spec.target_id = rqst.target_id;
>>>> 86 spec.command_id = rqst.command_id;
>>>> 87 spec.instance_id = rqst.instance_id;
>>>> 88 spec.flags = 0;
>>>> 89 spec.length = rqst.payload.length;
>>>> 90 spec.payload = NULL;
>>>> 91
>>>> 92 if (rqst.flags & SSAM_CDEV_REQUEST_HAS_RESPONSE)
>>>> 93 spec.flags |= SSAM_REQUEST_HAS_RESPONSE;
>>>> 94
>>>> 95 if (rqst.flags & SSAM_CDEV_REQUEST_UNSEQUENCED)
>>>> 96 spec.flags |= SSAM_REQUEST_UNSEQUENCED;
>>>> 97
>>>> 98 rsp.capacity = rqst.response.length;
>>>> 99 rsp.length = 0;
>>>> 100 rsp.pointer = NULL;
>>>> 101
>>>> 102 /* Get request payload from user-space. */
>>>> 103 if (spec.length) {
>>>> 104 if (!plddata) {
>>>> 105 ret = -EINVAL;
>>>> 106 goto out;
>>>> 107 }
>>>> 108
>>>>
>>>> CID: Untrusted allocation size (TAINTED_SCALAR)
>>>> 8. tainted_data: Passing tainted expression spec.length to
>>>> kzalloc,
>>>> which uses it as an allocation size
>>>>
>>>> 109 spec.payload = kzalloc(spec.length, GFP_KERNEL);
>>>
>>> I assume a constraint on the maximum length will fix this?
>>
>> I believe so, it's unsigned so just an upper size check will be required
>> to silence this static analysis warning. Mind you, you may want a size
>> that is the full u16 max of 65535, so in that case the check is not
>> required.
>
> Right, the theoretical maximum payload (spec.length) and response size
> allowed by the Surface Aggregator SSH protocol is 'U16_MAX -
> sizeof(struct ssh_command)' (not that anything this size should ever be
> allocated in any normal case). Meaning it is (slightly) smaller than
> U16_MAX, but I'm not sure if it warrants a check here. The payload size
> is later validated by ssam_request_sync(), so it does only affect the
> allocation here (the response is just an output buffer and may be of
> arbitrary size).
>
> I think the limit imposed by having u16 as user-input should be enough.
Sounds good to me.
> I can still add an explicit check here if that is preferred, but I could
> also add a comment explaining that this should be safe.
Re: platform/surface: Add Surface Aggregator user-space interface (static analysis issues)
On 11/01/2021 13:55, Maximilian Luz wrote:
> On 1/11/21 1:12 PM, Colin Ian King wrote:
>> Hi Maximilian,
>>
>> Static analysis of linux-next with Coverity has found several issues
>> with the following commit:
>>
>> commit 178f6ab77e617c984d6520b92e747075a12676ff
>> Author: Maximilian Luz
>> Date: Mon Dec 21 19:39:58 2020 +0100
>>
>> platform/surface: Add Surface Aggregator user-space interface
>>
>> The analysis is as follows:
>>
>> 65static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long
>> arg)
>> 66{
>> 67 struct ssam_cdev_request __user *r;
>> 68 struct ssam_cdev_request rqst;
>>
>> 1. var_decl: Declaring variable spec without initializer.
>>
>> 69 struct ssam_request spec;
>> 70 struct ssam_response rsp;
>> 71 const void __user *plddata;
>> 72 void __user *rspdata;
>> 73 int status = 0, ret = 0, tmp;
>> 74
>> 75 r = (struct ssam_cdev_request __user *)arg;
>> 76 ret = copy_struct_from_user(, sizeof(rqst), r,
>> sizeof(*r));
>>
>> 2. Condition ret, taking true branch.
>>
>> 77 if (ret)
>>
>> 3. Jumping to label out.
>>
>> 78 goto out;
>>
>> 79
>> 80 plddata = u64_to_user_ptr(rqst.payload.data);
>> 81 rspdata = u64_to_user_ptr(rqst.response.data);
>> 82
>> 83 /* Setup basic request fields. */
>> 84 spec.target_category = rqst.target_category;
>> 85 spec.target_id = rqst.target_id;
>> 86 spec.command_id = rqst.command_id;
>> 87 spec.instance_id = rqst.instance_id;
>> 88 spec.flags = 0;
>> 89 spec.length = rqst.payload.length;
>> 90 spec.payload = NULL;
>> 91
>> 92 if (rqst.flags & SSAM_CDEV_REQUEST_HAS_RESPONSE)
>> 93 spec.flags |= SSAM_REQUEST_HAS_RESPONSE;
>> 94
>> 95 if (rqst.flags & SSAM_CDEV_REQUEST_UNSEQUENCED)
>> 96 spec.flags |= SSAM_REQUEST_UNSEQUENCED;
>> 97
>> 98 rsp.capacity = rqst.response.length;
>> 99 rsp.length = 0;
>> 100 rsp.pointer = NULL;
>> 101
>> 102 /* Get request payload from user-space. */
>> 103 if (spec.length) {
>> 104 if (!plddata) {
>> 105 ret = -EINVAL;
>> 106 goto out;
>> 107 }
>> 108
>>
>> CID: Untrusted allocation size (TAINTED_SCALAR)
>> 8. tainted_data: Passing tainted expression spec.length to kzalloc,
>> which uses it as an allocation size
>>
>> 109 spec.payload = kzalloc(spec.length, GFP_KERNEL);
>
> I assume a constraint on the maximum length will fix this?
I believe so, it's unsigned so just an upper size check will be required
to silence this static analysis warning. Mind you, you may want a size
that is the full u16 max of 65535, so in that case the check is not
required.
>
>> 110 if (!spec.payload) {
>> 111 ret = -ENOMEM;
>> 112 goto out;
>> 113 }
>> 114
>> 115 if (copy_from_user((void *)spec.payload, plddata,
>> spec.length)) {
>> 116 ret = -EFAULT;
>> 117 goto out;
>> 118 }
>> 119 }
>> 120
>> 121 /* Allocate response buffer. */
>> 122 if (rsp.capacity) {
>> 123 if (!rspdata) {
>> 124 ret = -EINVAL;
>> 125 goto out;
>> 126 }
>> 127
>>
>> CID: Untrusted allocation size (TAINTED_SCALAR)
>> 12. tainted_data: Passing tainted expression rsp.capacity to kzalloc,
>> which uses it as an allocation size
>>
>> 128 rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL);
>> 129 if (!rsp.pointer) {
>> 130 ret = -ENOMEM;
>> 131 goto out;
>> 132 }
>> 133 }
>> 134
>> 135 /* Perform request. */
>> 136 status = ssam_request_sync(cdev->ctrl, , );
>> 137 if (status)
>> 138 goto out;
>> 139
>> 140 /* Copy response to user-space. */
>> 141 if (rsp.length && copy_to_user(rspdata, rsp.pointer,
>> rsp.length))
>> 142 ret = -EFAULT;
>
re: platform/surface: Add Surface Aggregator user-space interface (static analysis issues)
Hi Maximilian,
Static analysis of linux-next with Coverity has found several issues
with the following commit:
commit 178f6ab77e617c984d6520b92e747075a12676ff
Author: Maximilian Luz
Date: Mon Dec 21 19:39:58 2020 +0100
platform/surface: Add Surface Aggregator user-space interface
The analysis is as follows:
65static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
66{
67struct ssam_cdev_request __user *r;
68struct ssam_cdev_request rqst;
1. var_decl: Declaring variable spec without initializer.
69struct ssam_request spec;
70struct ssam_response rsp;
71const void __user *plddata;
72void __user *rspdata;
73int status = 0, ret = 0, tmp;
74
75r = (struct ssam_cdev_request __user *)arg;
76ret = copy_struct_from_user(, sizeof(rqst), r, sizeof(*r));
2. Condition ret, taking true branch.
77if (ret)
3. Jumping to label out.
78goto out;
79
80plddata = u64_to_user_ptr(rqst.payload.data);
81rspdata = u64_to_user_ptr(rqst.response.data);
82
83/* Setup basic request fields. */
84spec.target_category = rqst.target_category;
85spec.target_id = rqst.target_id;
86spec.command_id = rqst.command_id;
87spec.instance_id = rqst.instance_id;
88spec.flags = 0;
89spec.length = rqst.payload.length;
90spec.payload = NULL;
91
92if (rqst.flags & SSAM_CDEV_REQUEST_HAS_RESPONSE)
93spec.flags |= SSAM_REQUEST_HAS_RESPONSE;
94
95if (rqst.flags & SSAM_CDEV_REQUEST_UNSEQUENCED)
96spec.flags |= SSAM_REQUEST_UNSEQUENCED;
97
98rsp.capacity = rqst.response.length;
99rsp.length = 0;
100rsp.pointer = NULL;
101
102/* Get request payload from user-space. */
103if (spec.length) {
104if (!plddata) {
105ret = -EINVAL;
106goto out;
107}
108
CID: Untrusted allocation size (TAINTED_SCALAR)
8. tainted_data: Passing tainted expression spec.length to kzalloc,
which uses it as an allocation size
109spec.payload = kzalloc(spec.length, GFP_KERNEL);
110if (!spec.payload) {
111ret = -ENOMEM;
112goto out;
113}
114
115if (copy_from_user((void *)spec.payload, plddata,
spec.length)) {
116ret = -EFAULT;
117goto out;
118}
119}
120
121/* Allocate response buffer. */
122if (rsp.capacity) {
123if (!rspdata) {
124ret = -EINVAL;
125goto out;
126}
127
CID: Untrusted allocation size (TAINTED_SCALAR)
12. tainted_data: Passing tainted expression rsp.capacity to kzalloc,
which uses it as an allocation size
128rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL);
129if (!rsp.pointer) {
130ret = -ENOMEM;
131goto out;
132}
133}
134
135/* Perform request. */
136status = ssam_request_sync(cdev->ctrl, , );
137if (status)
138goto out;
139
140/* Copy response to user-space. */
141if (rsp.length && copy_to_user(rspdata, rsp.pointer, rsp.length))
142ret = -EFAULT;
143
144out:
145/* Always try to set response-length and status. */
CID: Uninitialized pointer read (UNINIT)
Using uninitialized value rsp.length
146tmp = put_user(rsp.length, >response.length);
4. Condition tmp, taking true branch.
147if (tmp)
148ret = tmp;
149
150tmp = put_user(status, >status);
5. Condition tmp, taking true branch.
151if (tmp)
152ret = tmp;
153
154/* Cleanup. */
CID: Uninitialized pointer read (UNINIT)
6. uninit_use_in_call: Using uninitialized value spec.payload when
calling kfree.
155kfree(spec.payload);
CID: Uninitialized pointer read (UNINIT)
uninit_use_in_call: Using uninitialized value rsp.pointer when
calling kfree
156kfree(rsp.pointer);
157
158return ret;
Colin
Re: [PATCH v2] f2fs: fix null page reference in redirty_blocks
On 05/01/2021 04:16, Daeho Jeong wrote:
> From: Daeho Jeong
>
> Fixed null page reference when find_lock_page() fails in
> redirty_blocks().
>
> Signed-off-by: Daeho Jeong
> Reported-by: Colin Ian King
> Fixes: 5fdb322ff2c2 ("f2fs: add F2FS_IOC_DECOMPRESS_FILE and
> F2FS_IOC_COMPRESS_FILE")
> ---
> v2: changed error value and break the loop when error occurs
> ---
> fs/f2fs/file.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 9e5275716be8..d27173c24391 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -4060,8 +4060,10 @@ static int redirty_blocks(struct inode *inode, pgoff_t
> page_idx, int len)
>
> for (i = 0; i < page_len; i++, redirty_idx++) {
> page = find_lock_page(mapping, redirty_idx);
> - if (!page)
> - ret = -ENOENT;
> + if (!page) {
> + ret = -ENOMEM;
> + break;
> + }
> set_page_dirty(page);
> f2fs_put_page(page, 1);
> f2fs_put_page(page, 0);
>
Thanks, looks good to me.
Reviewed-by: Colin Ian King
re: f2fs: add F2FS_IOC_DECOMPRESS_FILE and F2FS_IOC_COMPRESS_FILE
Hi,
Static analysis using Coverity has detected a potential null pointer
dereference after a null check in the following commit:
commit 5fdb322ff2c2b4ad519f490dcb7ebb96c5439af7
Author: Daeho Jeong
Date: Thu Dec 3 15:56:15 2020 +0900
f2fs: add F2FS_IOC_DECOMPRESS_FILE and F2FS_IOC_COMPRESS_FILE
The analysis is as follows:
4025 static int redirty_blocks(struct inode *inode, pgoff_t page_idx,
int len)
4026 {
4027DEFINE_READAHEAD(ractl, NULL, inode->i_mapping, page_idx);
4028struct address_space *mapping = inode->i_mapping;
4029struct page *page;
4030pgoff_t redirty_idx = page_idx;
4031int i, page_len = 0, ret = 0;
4032
4033page_cache_ra_unbounded(, len, 0);
4034
1. Condition i < len, taking true branch.
4. Condition i < len, taking true branch.
4035for (i = 0; i < len; i++, page_idx++) {
4036page = read_cache_page(mapping, page_idx, NULL, NULL);
2. Condition IS_ERR(page), taking false branch.
5. Condition IS_ERR(page), taking true branch.
4037if (IS_ERR(page)) {
4038ret = PTR_ERR(page);
6. Breaking from loop.
4039break;
4040}
4041page_len++;
3. Jumping back to the beginning of the loop.
4042}
4043
7. Condition i < page_len, taking true branch.
4044for (i = 0; i < page_len; i++, redirty_idx++) {
4045page = find_lock_page(mapping, redirty_idx);
8. Condition !page, taking true branch.
9. var_compare_op: Comparing page to null implies that page might be
null.
4046if (!page)
4047ret = -ENOENT;
Dereference after null check (FORWARD_NULL)
10. var_deref_model: Passing null pointer page to set_page_dirty,
which dereferences it.
4048set_page_dirty(page);
4049f2fs_put_page(page, 1);
4050f2fs_put_page(page, 0);
4051}
4052
4053return ret;
4054 }
The !page check on line 4046 sets ret appropriately but we have a
following call to set_page_dirty on a null page that causes the error.
Not sure how this should be fixed, should the check bail out immediately
or just avoid the following set_page_dirty anf f2fs_put_page calls?
Colin
[tip: timers/urgent] timekeeping: Fix spelling mistake in Kconfig "fullfill" -> "fulfill"
The following commit has been merged into the timers/urgent branch of tip: Commit-ID: f6f5cd840ae782680c5e94048c72420e4e6857f9 Gitweb: https://git.kernel.org/tip/f6f5cd840ae782680c5e94048c72420e4e6857f9 Author:Colin Ian King AuthorDate:Thu, 17 Dec 2020 17:17:05 Committer: Thomas Gleixner CommitterDate: Fri, 18 Dec 2020 23:15:00 +01:00 timekeeping: Fix spelling mistake in Kconfig "fullfill" -> "fulfill" There is a spelling mistake in the Kconfig help text. Fix it. Signed-off-by: Colin Ian King Signed-off-by: Thomas Gleixner Acked-by: Linus Walleij Link: https://lore.kernel.org/r/20201217171705.57586-1-colin.k...@canonical.com --- kernel/time/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig index a09b1d6..64051f4 100644 --- a/kernel/time/Kconfig +++ b/kernel/time/Kconfig @@ -141,7 +141,7 @@ config CONTEXT_TRACKING_FORCE dynticks working. This option stands for testing when an arch implements the - context tracking backend but doesn't yet fullfill all the + context tracking backend but doesn't yet fulfill all the requirements to make the full dynticks feature working. Without the full dynticks, there is no way to test the support for context tracking and the subsystems that rely on it: RCU
NAK: [PATCH] wilc1000: fix spelling mistake in Kconfig "devision" -> "division"
On 16/12/2020 11:54, Colin King wrote: > From: Colin Ian King > > There is a spelling mistake in the Kconfig help text. Fix it. > > Signed-off-by: Colin Ian King > --- > drivers/net/ethernet/ni/Kconfig | 2 +- > drivers/net/wireless/microchip/wilc1000/Kconfig | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/ni/Kconfig b/drivers/net/ethernet/ni/Kconfig > index 01229190132d..dcfbfa516e67 100644 > --- a/drivers/net/ethernet/ni/Kconfig > +++ b/drivers/net/ethernet/ni/Kconfig > @@ -1,6 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0-only > # > -# National Instuments network device configuration > +# National Instruments network device configuration > # > > config NET_VENDOR_NI > diff --git a/drivers/net/wireless/microchip/wilc1000/Kconfig > b/drivers/net/wireless/microchip/wilc1000/Kconfig > index 80c92e8bf8a5..7f15e42602dd 100644 > --- a/drivers/net/wireless/microchip/wilc1000/Kconfig > +++ b/drivers/net/wireless/microchip/wilc1000/Kconfig > @@ -44,4 +44,4 @@ config WILC1000_HW_OOB_INTR > chipset. This OOB interrupt is intended to provide a faster interrupt > mechanism for SDIO host controllers that don't support SDIO interrupt. > Select this option If the SDIO host controller in your platform > - doesn't support SDIO time devision interrupt. > + doesn't support SDIO time division interrupt. > Messed this up. V2 coming soon.
re: scsi: ufs: Serialize eh_work with system PM events and async scan
Hi,
Static analysis on linux-next with Coverity had found a potential null
pointer dereference issue in the following commit:
commit 88a92d6ae4fe09b2b27781178c5c9432d27b1ffb
Author: Can Guo
Date: Wed Dec 2 04:04:01 2020 -0800
scsi: ufs: Serialize eh_work with system PM events and async scan
The analysis by Coverity is as follows:
8929 int ufshcd_system_suspend(struct ufs_hba *hba)
8930 {
8931int ret = 0;
8932ktime_t start = ktime_get();
8933
deref_ptr_in_call: Dereferencing pointer hba.
8934down(>eh_sem);
Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking hba suggests that it may be null, but
it has already been dereferenced on all paths leading to the check.
8935if (!hba || !hba->is_powered)
8936return 0;
Seeing that the down lock has been added by the commit it suggests the
commit overlooks the fact that hba may potentially be null. Not sure if
hba can be null, so I'm not sure if this is a real bug or a false positive.
Colin
Re: [PATCH][next] seg6: fix unintentional integer overflow on left shift
On 07/12/2020 19:59, Andrea Mayer wrote:
> On Mon, 7 Dec 2020 14:45:03 +
> Colin King wrote:
>
>> From: Colin Ian King
>>
>> Shifting the integer value 1 is evaluated using 32-bit arithmetic
>> and then used in an expression that expects a unsigned long value
>> leads to a potential integer overflow. Fix this by using the BIT
>> macro to perform the shift to avoid the overflow.
>>
>> Addresses-Coverity: ("Uninitentional integer overflow")
>> Fixes: 964adce526a4 ("seg6: improve management of behavior attributes")
>> Signed-off-by: Colin Ian King
>> ---
>> net/ipv6/seg6_local.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
>> index b07f7c1c82a4..d68de8cd1207 100644
>> --- a/net/ipv6/seg6_local.c
>> +++ b/net/ipv6/seg6_local.c
>> @@ -1366,7 +1366,7 @@ static void __destroy_attrs(unsigned long
>> parsed_attrs, int max_parsed,
>> * attribute; otherwise, we call the destroy() callback.
>> */
>> for (i = 0; i < max_parsed; ++i) {
>> -if (!(parsed_attrs & (1 << i)))
>> +if (!(parsed_attrs & BIT(i)))
>> continue;
>>
>> param = _action_params[i];
>> --
>> 2.29.2
>>
>
> Hi Colin,
> thanks for the fix. I've just given a look a the whole seg6_local.c code and I
> found that such issues is present in other parts of the code.
>
> If we agree, I can make a fix which explicitly eliminates the several (1 << i)
> in favor of BIT(i).
Sounds good to me.
Colin
>
> Andrea
>
Re: media: i2c: add OV02A10 image sensor driver
On 03/12/2020 18:10, Andy Shevchenko wrote:
> On Thu, Dec 3, 2020 at 8:03 PM Colin Ian King
> wrote:
>
>> Static analysis on linux-next with Coverity has detected an issue with
>> the following commit:
>
> If you want to fix it properly, see my comments below...
>
>> 529 static int ov02a10_s_stream(struct v4l2_subdev *sd, int on)
>> 530 {
>> 531struct ov02a10 *ov02a10 = to_ov02a10(sd);
>> 532struct i2c_client *client =
>> v4l2_get_subdevdata(>subdev);
>>
>>1. var_decl: Declaring variable ret without initializer.
>>
>> 533int ret;
>> 534
>> 535mutex_lock(>mutex);
>> 536
>>
>>2. Condition ov02a10->streaming == on, taking true branch.
>>
>> 537if (ov02a10->streaming == on)
>>
>>3. Jumping to label unlock_and_return.
>>
>> 538goto unlock_and_return;
>> 539
>> 540if (on) {
>> 541ret = pm_runtime_get_sync(>dev);
>> 542if (ret < 0) {
>
>> 543pm_runtime_put_noidle(>dev);
>> 544goto unlock_and_return;
>
> Instead of two above:
>goto err_rpm_put;
>
>> 545}
>> 546
>> 547ret = __ov02a10_start_stream(ov02a10);
>> 548if (ret) {
>> 549__ov02a10_stop_stream(ov02a10);
>> 550ov02a10->streaming = !on;
>> 551goto err_rpm_put;
>> 552}
>> 553} else {
>> 554__ov02a10_stop_stream(ov02a10);
>> 555pm_runtime_put(>dev);
>> 556}
>> 557
>> 558ov02a10->streaming = on;
>
> (1)
>
>> 559mutex_unlock(>mutex);
>> 560
>> 561return 0;
>> 562
>> 563 err_rpm_put:
>> 564pm_runtime_put(>dev);
>
>> 565 unlock_and_return:
>
> Should be moved to (1).
>
>> 566mutex_unlock(>mutex);
>> 567
>>
>> Uninitialized scalar variable (UNINIT)
>> 4. uninit_use: Using uninitialized value ret.
>>
>> 568return ret;
>> 569 }
>>
>> Variable ret has not been initialized, so the error return value is a
>> garbage value. It should be initialized with some appropriate negative
>> error code, or ret could be removed and the return should return a
>> literal value of a error code.
>>
>> I was unsure what value is appropriate to fix this, so instead I'm
>> reporting this issue.
>
Not sure I fully understand how that fixes it. Given that ret is
currently not initialized when we hit:
if (ov02a10->streaming == on)
goto unlock_and_return;
either we initialize ret to 0 at the start of the function, or do:
if (ov02a10->streaming == on) {
ret = 0;
goto unlock_and_return;
}
(assuming the intention is to return zero for this specific case).
Colin
re: media: i2c: add OV02A10 image sensor driver
Hi,
Static analysis on linux-next with Coverity has detected an issue with
the following commit:
529 static int ov02a10_s_stream(struct v4l2_subdev *sd, int on)
530 {
531struct ov02a10 *ov02a10 = to_ov02a10(sd);
532struct i2c_client *client =
v4l2_get_subdevdata(>subdev);
1. var_decl: Declaring variable ret without initializer.
533int ret;
534
535mutex_lock(>mutex);
536
2. Condition ov02a10->streaming == on, taking true branch.
537if (ov02a10->streaming == on)
3. Jumping to label unlock_and_return.
538goto unlock_and_return;
539
540if (on) {
541ret = pm_runtime_get_sync(>dev);
542if (ret < 0) {
543pm_runtime_put_noidle(>dev);
544goto unlock_and_return;
545}
546
547ret = __ov02a10_start_stream(ov02a10);
548if (ret) {
549__ov02a10_stop_stream(ov02a10);
550ov02a10->streaming = !on;
551goto err_rpm_put;
552}
553} else {
554__ov02a10_stop_stream(ov02a10);
555pm_runtime_put(>dev);
556}
557
558ov02a10->streaming = on;
559mutex_unlock(>mutex);
560
561return 0;
562
563 err_rpm_put:
564pm_runtime_put(>dev);
565 unlock_and_return:
566mutex_unlock(>mutex);
567
Uninitialized scalar variable (UNINIT)
4. uninit_use: Using uninitialized value ret.
568return ret;
569 }
Variable ret has not been initialized, so the error return value is a
garbage value. It should be initialized with some appropriate negative
error code, or ret could be removed and the return should return a
literal value of a error code.
I was unsure what value is appropriate to fix this, so instead I'm
reporting this issue.
Colin
Re: ACK: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
On 27/11/2020 19:29, Ard Biesheuvel wrote:
> On Fri, 27 Nov 2020 at 20:28, Colin Ian King wrote:
>>
>> On 27/11/2020 19:20, Heinrich Schuchardt wrote:
>>> Since the UEFI 2.8A specification the UEFI enabled firmware provides a
>>> configuration table EFI_RT_PROPERTIES_TABLE which indicates which runtime
>>> services are enabled. The EFI stub reads this table and saves the value of
>>> the field RuntimeServicesSupported internally.
>>>
>>> The Firmware Test Suite requires the value to determine if UEFI runtime
>>> services are correctly implemented.
>>>
>>> With this patch an IOCTL call is provided to read the value of the field
>>> RuntimeServicesSupported, e.g.
>>>
>>> #define EFI_RUNTIME_GET_SUPPORTED_MASK \
>>> _IOR('p', 0x0C, unsigned int)
>>> unsigned int mask;
>>> fd = open("/dev/efi_test", O_RDWR);
>>> ret = ioctl(fd, EFI_RUNTIME_GET_SUPPORTED_MASK, );
>>>
>>> Signed-off-by: Heinrich Schuchardt
>>> ---
>>> drivers/firmware/efi/test/efi_test.c | 16
>>> drivers/firmware/efi/test/efi_test.h | 3 +++
>>> 2 files changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/firmware/efi/test/efi_test.c
>>> b/drivers/firmware/efi/test/efi_test.c
>>> index ddf9eae396fe..47d67bb0a516 100644
>>> --- a/drivers/firmware/efi/test/efi_test.c
>>> +++ b/drivers/firmware/efi/test/efi_test.c
>>> @@ -663,6 +663,19 @@ static long efi_runtime_query_capsulecaps(unsigned
>>> long arg)
>>> return rv;
>>> }
>>>
>>> +static long efi_runtime_get_supported_mask(unsigned long arg)
>>> +{
>>> + unsigned int __user *supported_mask;
>>> + int rv = 0;
>>> +
>>> + supported_mask = (unsigned int *)arg;
>>> +
>>> + if (put_user(efi.runtime_supported_mask, supported_mask))
>>> + rv = -EFAULT;
>>> +
>>> + return rv;
>>> +}
>>> +
>>> static long efi_test_ioctl(struct file *file, unsigned int cmd,
>>> unsigned long arg)
>>> {
>>> @@ -699,6 +712,9 @@ static long efi_test_ioctl(struct file *file, unsigned
>>> int cmd,
>>>
>>> case EFI_RUNTIME_RESET_SYSTEM:
>>> return efi_runtime_reset_system(arg);
>>> +
>>> + case EFI_RUNTIME_GET_SUPPORTED_MASK:
>>> + return efi_runtime_get_supported_mask(arg);
>>> }
>>>
>>> return -ENOTTY;
>>> diff --git a/drivers/firmware/efi/test/efi_test.h
>>> b/drivers/firmware/efi/test/efi_test.h
>>> index f2446aa1c2e3..117349e57993 100644
>>> --- a/drivers/firmware/efi/test/efi_test.h
>>> +++ b/drivers/firmware/efi/test/efi_test.h
>>> @@ -118,4 +118,7 @@ struct efi_resetsystem {
>>> #define EFI_RUNTIME_RESET_SYSTEM \
>>> _IOW('p', 0x0B, struct efi_resetsystem)
>>>
>>> +#define EFI_RUNTIME_GET_SUPPORTED_MASK \
>>> + _IOR('p', 0x0C, unsigned int)
>>> +
>>> #endif /* _DRIVERS_FIRMWARE_EFI_TEST_H_ */
>>> --
>>> 2.29.2
>>>
>>
>> Looks good to me. Thanks Heinrich.
>>
>> The EFI driver needs to be also updated in the linux kernel - has that
>> fix been submitted or do you require the fwts team to do that?
Oops. It's been a lot week :-(
>>
>
> This /is/ the EFI driver.
>
> I'll take this as an acked-by but I'd like Ivan to chime in as well.
>
+1
ACK: [PATCH 1/1] efi/efi_test: read RuntimeServicesSupported
On 27/11/2020 19:20, Heinrich Schuchardt wrote:
> Since the UEFI 2.8A specification the UEFI enabled firmware provides a
> configuration table EFI_RT_PROPERTIES_TABLE which indicates which runtime
> services are enabled. The EFI stub reads this table and saves the value of
> the field RuntimeServicesSupported internally.
>
> The Firmware Test Suite requires the value to determine if UEFI runtime
> services are correctly implemented.
>
> With this patch an IOCTL call is provided to read the value of the field
> RuntimeServicesSupported, e.g.
>
> #define EFI_RUNTIME_GET_SUPPORTED_MASK \
> _IOR('p', 0x0C, unsigned int)
> unsigned int mask;
> fd = open("/dev/efi_test", O_RDWR);
> ret = ioctl(fd, EFI_RUNTIME_GET_SUPPORTED_MASK, );
>
> Signed-off-by: Heinrich Schuchardt
> ---
> drivers/firmware/efi/test/efi_test.c | 16
> drivers/firmware/efi/test/efi_test.h | 3 +++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/firmware/efi/test/efi_test.c
> b/drivers/firmware/efi/test/efi_test.c
> index ddf9eae396fe..47d67bb0a516 100644
> --- a/drivers/firmware/efi/test/efi_test.c
> +++ b/drivers/firmware/efi/test/efi_test.c
> @@ -663,6 +663,19 @@ static long efi_runtime_query_capsulecaps(unsigned long
> arg)
> return rv;
> }
>
> +static long efi_runtime_get_supported_mask(unsigned long arg)
> +{
> + unsigned int __user *supported_mask;
> + int rv = 0;
> +
> + supported_mask = (unsigned int *)arg;
> +
> + if (put_user(efi.runtime_supported_mask, supported_mask))
> + rv = -EFAULT;
> +
> + return rv;
> +}
> +
> static long efi_test_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -699,6 +712,9 @@ static long efi_test_ioctl(struct file *file, unsigned
> int cmd,
>
> case EFI_RUNTIME_RESET_SYSTEM:
> return efi_runtime_reset_system(arg);
> +
> + case EFI_RUNTIME_GET_SUPPORTED_MASK:
> + return efi_runtime_get_supported_mask(arg);
> }
>
> return -ENOTTY;
> diff --git a/drivers/firmware/efi/test/efi_test.h
> b/drivers/firmware/efi/test/efi_test.h
> index f2446aa1c2e3..117349e57993 100644
> --- a/drivers/firmware/efi/test/efi_test.h
> +++ b/drivers/firmware/efi/test/efi_test.h
> @@ -118,4 +118,7 @@ struct efi_resetsystem {
> #define EFI_RUNTIME_RESET_SYSTEM \
> _IOW('p', 0x0B, struct efi_resetsystem)
>
> +#define EFI_RUNTIME_GET_SUPPORTED_MASK \
> + _IOR('p', 0x0C, unsigned int)
> +
> #endif /* _DRIVERS_FIRMWARE_EFI_TEST_H_ */
> --
> 2.29.2
>
Looks good to me. Thanks Heinrich.
The EFI driver needs to be also updated in the linux kernel - has that
fix been submitted or do you require the fwts team to do that?
Colin
Re: [PATCH] dma-mapping: Fix sizeof() mismatch on tsk allocation
On 25/11/2020 18:29, Christoph Hellwig wrote: > I'll fold this one in as well. > OK, so two SoB's disappear?
Re: [PATCH][next] drm/kmb: fix array out-of-bounds writes to kmb->plane_status[]
On 16/11/2020 16:53, Chrisanthus, Anitha wrote:
> Hi Sam and Colin,
>
>> -Original Message-
>> From: Sam Ravnborg
>> Sent: Friday, November 13, 2020 10:02 AM
>> To: Colin Ian King
>> Cc: Chrisanthus, Anitha ; Dea, Edmund J
>> ; David Airlie ; Daniel Vetter
>> ; dri-de...@lists.freedesktop.org; kernel-
>> janit...@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH][next] drm/kmb: fix array out-of-bounds writes to kmb-
>>> plane_status[]
>>
>> Hi Colin.
>> On Fri, Nov 13, 2020 at 03:04:34PM +, Colin Ian King wrote:
>>> On 13/11/2020 14:55, Sam Ravnborg wrote:
>>>> Hi Colin.
>>>>
>>>> On Fri, Nov 13, 2020 at 12:01:21PM +, Colin King wrote:
>>>>> From: Colin Ian King
>>>>>
>>>>> Writes to elements in the kmb->plane_status array in function
>>>>> kmb_plane_atomic_disable are overrunning the array when plane_id is
>>>>> more than 1 because currently the array is KMB_MAX_PLANES elements
>>>>> in size and this is currently #defined as 1. Fix this by defining
>>>>> KMB_MAX_PLANES to 4.
>>>>
>>>> I fail to follow you here.
>>>> In kmb_plane_init() only one plane is allocated - with id set to 0.
>>>> So for now only one plane is allocated thus kmb_plane_atomic_disable()
>>>> is only called for this plane.
>>>>
>>>> With your change we will start allocating four planes, something that is
>>>> not tested.
>>>>
>>>> Do I miss something?
>>>>
>>>>Sam
>>>>
>>>
>>> The static analysis from coverity on linux-next suggested that there was
>>> an array overflow as follows:
>>>
>>> 108 static void kmb_plane_atomic_disable(struct drm_plane *plane,
>>> 109 struct drm_plane_state *state)
>>> 110 {
>>>
>>>1. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>
>>> 111struct kmb_plane *kmb_plane = to_kmb_plane(plane);
>>>
>>>2. assignment: Assigning: plane_id = kmb_plane->id.
>>>
>>> 112int plane_id = kmb_plane->id;
>>> 113struct kmb_drm_private *kmb;
>>> 114
>>> 115kmb = to_kmb(plane->dev);
>>> 116
>>>
>>>3. Switch case value LAYER_3.
>>>
>>> 117switch (plane_id) {
>>> 118case LAYER_0:
>>> 119kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL1_ENABLE;
>>> 120break;
>>
>> With the current code this is the only case that hits.
>> So coverity is right that if we hit other cases that would result in a
>> bug. But kmb_plane->id will for now not have other values than 0.
>>
>> So it is a subtle false positive.
>> There is some "dead" code here - but this is in preparation for more
>> than one layer and we will keep the code for now, unless Anitha chimes
>> in and says otherwise.
>
> Thanks Sam, I was out on Friday. Agree with Sam, let's keep the current code
> for now. Kmb->plane_id will not have non-zero values now.
> Only one plane is supported and tested currently, the extra code is in
> preparation for multiple planes.
Thanks for the clarification. Apologies for the noise.
>
> Thanks,
> Anitha
>>
>> Sam
>>
>>> 121case LAYER_1:
>>>
>>>(#2 of 4): Out-of-bounds write (OVERRUN)
>>>
>>> 122kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL2_ENABLE;
>>> 123break;
>>> 124case LAYER_2:
>>>
>>>(#3 of 4): Out-of-bounds write (OVERRUN)
>>>
>>> 125kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL1_ENABLE;
>>> 126break;
>>>
>>>4. equality_cond: Jumping to case LAYER_3.
>>>
>>> 127case LAYER_3:
>>>
>>>(#1 of 4): Out-of-bounds write (OVERRUN)
>>>5. overrun-local: Overrunning array kmb->plane_status of 1 8-byte
>>> elements at element index 3 (byte offset 31) using index plane_id (which
>>> evaluates to 3).
>>>
>>> 128kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL2_ENABLE;
>>> 129break;
>>> 130}
>>> 131
>>>
>>>(#4 of 4): Out-of-bounds write (OVERRUN)
>>>
>>> 132kmb->plane_status[plane_id].disable = true;
>>> 133 }
>>> 134
>>>
>>> So it seems the assignments to kmb->plane_status[plane_id] are
>>> overrunning the array since plane_status is allocated as 1 element and
>>> yet plane_id can be 0..3
>>>
>>> I could be misunderstanding this, or it may be a false positive.
>>>
>>> Colin
Re: [PATCH][next] drm/atomic: avoid null pointer dereference on pointer crtc
On 16/11/2020 11:08, Simon Ser wrote:
> On Monday, November 16, 2020 12:03 PM, Colin King
> wrote:
>
>> From: Colin Ian King colin.k...@canonical.com
>>
>> Since moving to the new debug helper functions we now have a debug message
>> that dereferences crtc to print a kernel debug message when crtc is null
>> and so this debug message will now cause a null pointer dereference. Since
>> this is a debug message it probably is just simplest to fix this by just
>> removing the debug message altogether.
>
> NACK. This removes the log altogether instead of fixing it.
>
> A fix has already been pushed to drm-misc-next: 0003b687ee6d ("drm: fix
> oops in drm_atomic_set_crtc_for_connector").
>
Good to see this has already been fixed. Thanks.
Colin
Re: [PATCH][V2] PCI: Fix a potential uninitentional integer overflow issue
On 14/11/2020 21:53, Bjorn Helgaas wrote:
> [+cc Dan]
>
> On Tue, Nov 10, 2020 at 10:10:48PM +, Colin King wrote:
>> From: Colin Ian King
>>
>> The shift of 1 by align_order is evaluated using 32 bit arithmetic
>> and the result is assigned to a resource_size_t type variable that
>> is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow
>> before widening issue by making the 1 a ULL.
>>
>> Addresses-Coverity: ("Unintentional integer overflow")
>> Fixes: 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel parameters
>> reusable")
>> Signed-off-by: Colin Ian King
>
> Applied to pci/misc for v5.11 with Logan's Reviewed-by and also the
> Fixes: correction.
>
> I first applied the patch below to bounds-check the alignment as noted
> by Dan.
>
>> ---
>>
>> V2: Use ULL instead of BIT_ULL(), fix spelling mistake and capitalize first
>> word of patch subject.
>>
>> ---
>> drivers/pci/pci.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 3ef63a101fa1..248044a7ef8c 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -6214,7 +6214,7 @@ static resource_size_t
>> pci_specified_resource_alignment(struct pci_dev *dev,
>> if (align_order == -1)
>> align = PAGE_SIZE;
>> else
>> -align = 1 << align_order;
>> +align = 1ULL << align_order;
>> break;
>> } else if (ret < 0) {
>> pr_err("PCI: Can't parse resource_alignment parameter:
>> %s\n",
>
> commit d6ca242c448f ("PCI: Bounds-check command-line resource alignment
> requests")
> Author: Bjorn Helgaas
> Date: Thu Nov 5 14:51:36 2020 -0600
>
> PCI: Bounds-check command-line resource alignment requests
>
> 32-bit BARs are limited to 2GB size (2^31). By extension, I assume 64-bit
> BARs are limited to 2^63 bytes. Limit the alignment requested by the
> "pci=resource_alignment=" command-line parameter to 2^63.
>
> Link: https://lore.kernel.org/r/20201007123045.GS4282@kadam
> Reported-by: Dan Carpenter
> Signed-off-by: Bjorn Helgaas
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8b9bea8ba751..26c1b2d0bacd 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6197,19 +6197,21 @@ static resource_size_t
> pci_specified_resource_alignment(struct pci_dev *dev,
> while (*p) {
> count = 0;
> if (sscanf(p, "%d%n", _order, ) == 1 &&
> - p[count] == '@') {
> + p[count] == '@') {
> p += count + 1;
> + if (align_order > 63) {
> + pr_err("PCI: Invalid requested alignment (order
> %d)\n",
> +align_order);
> + align_order = PAGE_SHIFT;
> + }
> } else {
> - align_order = -1;
> + align_order = PAGE_SHIFT;
> }
>
> ret = pci_dev_str_match(dev, p, );
> if (ret == 1) {
> *resize = true;
> - if (align_order == -1)
> - align = PAGE_SIZE;
> - else
> - align = 1 << align_order;
> + align = 1 << align_order;
> break;
> } else if (ret < 0) {
> pr_err("PCI: Can't parse resource_alignment parameter:
> %s\n",
>
Thanks.
Re: [PATCH][next] drm/kmb: fix array out-of-bounds writes to kmb->plane_status[]
On 13/11/2020 14:55, Sam Ravnborg wrote:
> Hi Colin.
>
> On Fri, Nov 13, 2020 at 12:01:21PM +, Colin King wrote:
>> From: Colin Ian King
>>
>> Writes to elements in the kmb->plane_status array in function
>> kmb_plane_atomic_disable are overrunning the array when plane_id is
>> more than 1 because currently the array is KMB_MAX_PLANES elements
>> in size and this is currently #defined as 1. Fix this by defining
>> KMB_MAX_PLANES to 4.
>
> I fail to follow you here.
> In kmb_plane_init() only one plane is allocated - with id set to 0.
> So for now only one plane is allocated thus kmb_plane_atomic_disable()
> is only called for this plane.
>
> With your change we will start allocating four planes, something that is
> not tested.
>
> Do I miss something?
>
> Sam
>
The static analysis from coverity on linux-next suggested that there was
an array overflow as follows:
108 static void kmb_plane_atomic_disable(struct drm_plane *plane,
109 struct drm_plane_state *state)
110 {
1. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
111struct kmb_plane *kmb_plane = to_kmb_plane(plane);
2. assignment: Assigning: plane_id = kmb_plane->id.
112int plane_id = kmb_plane->id;
113struct kmb_drm_private *kmb;
114
115kmb = to_kmb(plane->dev);
116
3. Switch case value LAYER_3.
117switch (plane_id) {
118case LAYER_0:
119kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL1_ENABLE;
120break;
121case LAYER_1:
(#2 of 4): Out-of-bounds write (OVERRUN)
122kmb->plane_status[plane_id].ctrl = LCD_CTRL_VL2_ENABLE;
123break;
124case LAYER_2:
(#3 of 4): Out-of-bounds write (OVERRUN)
125kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL1_ENABLE;
126break;
4. equality_cond: Jumping to case LAYER_3.
127case LAYER_3:
(#1 of 4): Out-of-bounds write (OVERRUN)
5. overrun-local: Overrunning array kmb->plane_status of 1 8-byte
elements at element index 3 (byte offset 31) using index plane_id (which
evaluates to 3).
128kmb->plane_status[plane_id].ctrl = LCD_CTRL_GL2_ENABLE;
129break;
130}
131
(#4 of 4): Out-of-bounds write (OVERRUN)
132kmb->plane_status[plane_id].disable = true;
133 }
134
So it seems the assignments to kmb->plane_status[plane_id] are
overrunning the array since plane_status is allocated as 1 element and
yet plane_id can be 0..3
I could be misunderstanding this, or it may be a false positive.
Colin
Re: [PATCH][next] mptcp: fix a dereference of pointer before msk is null checked.
On 11/11/2020 18:49, Mat Martineau wrote:
> On Mon, 9 Nov 2020, Colin King wrote:
>
>> From: Colin Ian King
>>
>> Currently the assignment of pointer net from the sock_net(sk) call
>> is potentially dereferencing a null pointer sk. sk points to the
>> same location as pointer msk and msk is being null checked after
>> the sock_net call. Fix this by calling sock_net after the null
>> check on pointer msk.
>>
>> Addresses-Coverity: ("Dereference before null check")
>> Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
>> Signed-off-by: Colin Ian King
>> ---
>> net/mptcp/pm_netlink.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>
> Hi Colin and Jakub -
>
> I noticed that the follow-up discussion on this patch didn't go to the
> netdev list, so patchwork did not get updated.
>
> This patch is superseded by the following, which already has a
> Reviewed-by tag from Matthieu:
>
> http://patchwork.ozlabs.org/project/netdev/patch/078a2ef5bdc4e3b2c25ef852461692001f426495.1604976945.git.geliangt...@gmail.com/
>
>
OK, thanks for letting me know. Good to see it got fixed!
Colin
>
> Thanks!
>
> --
> Mat Martineau
> Intel
[tip: sched/urgent] sched/debug: Fix memory corruption caused by multiple small reads of flags
The following commit has been merged into the sched/urgent branch of tip:
Commit-ID: 8d4d9c7b4333abccb3bf310d76ef7ea2edb9828f
Gitweb:
https://git.kernel.org/tip/8d4d9c7b4333abccb3bf310d76ef7ea2edb9828f
Author:Colin Ian King
AuthorDate:Thu, 29 Oct 2020 15:11:03
Committer: Peter Zijlstra
CommitterDate: Tue, 10 Nov 2020 18:38:49 +01:00
sched/debug: Fix memory corruption caused by multiple small reads of flags
Reading /proc/sys/kernel/sched_domain/cpu*/domain0/flags mutliple times
with small reads causes oopses with slub corruption issues because the kfree is
free'ing an offset from a previous allocation. Fix this by adding in a new
pointer 'buf' for the allocation and kfree and use the temporary pointer tmp
to handle memory copies of the buf offsets.
Fixes: 5b9f8ff7b320 ("sched/debug: Output SD flag names rather than their
values")
Reported-by: Jeff Bastian
Signed-off-by: Colin Ian King
Signed-off-by: Peter Zijlstra (Intel)
Reviewed-by: Valentin Schneider
Link: https://lkml.kernel.org/r/20201029151103.373410-1-colin.k...@canonical.com
---
kernel/sched/debug.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index 0655524..2357921 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -251,7 +251,7 @@ static int sd_ctl_doflags(struct ctl_table *table, int
write,
unsigned long flags = *(unsigned long *)table->data;
size_t data_size = 0;
size_t len = 0;
- char *tmp;
+ char *tmp, *buf;
int idx;
if (write)
@@ -269,17 +269,17 @@ static int sd_ctl_doflags(struct ctl_table *table, int
write,
return 0;
}
- tmp = kcalloc(data_size + 1, sizeof(*tmp), GFP_KERNEL);
- if (!tmp)
+ buf = kcalloc(data_size + 1, sizeof(*buf), GFP_KERNEL);
+ if (!buf)
return -ENOMEM;
for_each_set_bit(idx, , __SD_FLAG_CNT) {
char *name = sd_flag_debug[idx].name;
- len += snprintf(tmp + len, strlen(name) + 2, "%s ", name);
+ len += snprintf(buf + len, strlen(name) + 2, "%s ", name);
}
- tmp += *ppos;
+ tmp = buf + *ppos;
len -= *ppos;
if (len > *lenp)
@@ -294,7 +294,7 @@ static int sd_ctl_doflags(struct ctl_table *table, int
write,
*lenp = len;
*ppos += len;
- kfree(tmp);
+ kfree(buf);
return 0;
}
Re: [PATCH] PCI: fix a potential uninitentional integer overflow issue
On 10/11/2020 20:54, Bjorn Helgaas wrote:
> On Fri, Nov 06, 2020 at 11:04:19AM +0300, Dan Carpenter wrote:
>> On Thu, Nov 05, 2020 at 04:24:30PM -0600, Bjorn Helgaas wrote:
>>> On Wed, Oct 07, 2020 at 03:33:45PM +0300, Dan Carpenter wrote:
>>>> On Wed, Oct 07, 2020 at 12:46:15PM +0100, Colin King wrote:
>>>>> From: Colin Ian King
>>>>>
>>>>> The shift of 1 by align_order is evaluated using 32 bit arithmetic
>>>>> and the result is assigned to a resource_size_t type variable that
>>>>> is a 64 bit unsigned integer on 64 bit platforms. Fix an overflow
>>>>> before widening issue by using the BIT_ULL macro to perform the
>>>>> shift.
>>>>>
>>>>> Addresses-Coverity: ("Uninitentional integer overflow")
>>>
>>> s/Uninitentional/Unintentional/
>>> Also in subject (please also capitalize subject)
OK
>>>
>>> Doesn't Coverity also assign an ID number for this specific issue?
>>> Can you include that as well, e.g.,
I'm running this from an internal coverity scan, so the ID is not public.
>>>
>>> Addresses-Coverity-ID: 1226899 ("Unintentional integer overflow")
>>>
>>>>> Fixes: 07d8d7e57c28 ("PCI: Make specifying PCI devices in kernel
>>>>> parameters reusable")
>>>>> Signed-off-by: Colin Ian King
>>>>> ---
>>>>> drivers/pci/pci.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>> index 6d4d5a2f923d..1a5844d7af35 100644
>>>>> --- a/drivers/pci/pci.c
>>>>> +++ b/drivers/pci/pci.c
>>>>> @@ -6209,7 +6209,7 @@ static resource_size_t
>>>>> pci_specified_resource_alignment(struct pci_dev *dev,
>>>>> if (align_order == -1)
>>>>> align = PAGE_SIZE;
>>>>> else
>>>>> - align = 1 << align_order;
>>>>> + align = BIT_ULL(align_order);
>>>>
>>>> "align_order" comes from sscanf() so Smatch thinks it's not trusted.
>>>> Anything above 63 is undefined behavior. There should be a bounds check
>>>> on this but I don't know what the valid values of "align" are.
>>>
>>> The spec doesn't explicitly say what the size limit for 64-bit BARs
>>> is, but it does say 32-bit BARs can support up to 2GB (2^31). So I
>>> infer that 2^63 would be the limit for 64-bit BARs.
>>>
>>> What about something like the following? To me, BIT_ULL doesn't seem
>>> like an advantage over "1ULL << ", but maybe there's a reason to use
>>> it.
>>
>> The advantage of BIT_ULL() is that checkpatch and I think Coccinelle
>> will suggest using it. It's only recently where a few people have
>> complained (actually you're probably the second person) that BIT() is
>> sort of a weird thing to use for size variables.
>
> If that's the only reason, I definitely prefer "1ULL << align_order".
>
> BIT_ULL is just a pointless abstraction in this case.
>
OK. V2 Arriving later today
Colin
Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char
On 10/11/2020 18:38, Paul E. McKenney wrote:
> On Tue, Nov 10, 2020 at 03:34:05PM +0000, Colin Ian King wrote:
>> On 10/11/2020 15:24, Paul E. McKenney wrote:
>>> On Mon, Nov 09, 2020 at 11:57:15PM -0500, Paul Gortmaker wrote:
>>>>
>>>>
>>>> On 2020-11-09 8:07 p.m., Qian Cai wrote:
>>>>> On Mon, 2020-11-09 at 13:04 +, Colin King wrote:
>>>>>> From: Colin Ian King
>>>>>>
>>>>>> Currently the allocation of cpulist is based on the length of buf but
>>>>>> does
>>>>>> not include the addition end of string '\0' terminator. Static analysis
>>>>>> is
>>>>>> reporting this as a potential out-of-bounds access on cpulist. Fix this
>>>>>> by
>>>>>> allocating enough space for the additional '\0' terminator.
>>>>>>
>>>>>> Addresses-Coverity: ("Out-of-bounds access")
>>>>>> Fixes: 65987e67f7ff ("cpumask: add "last" alias for cpu list
>>>>>> specifications")
>>>>>
>>>>> Yeah, this bad commit also introduced KASAN errors everywhere and then
>>>>> will
>>>>> disable lockdep that makes our linux-next CI miserable. Confirmed that
>>>>> this
>>>>> patch will fix it.
>>>>
>>>> I appreciate the reports reminding me why I hate touching string handling.
>>>>
>>>> But let us not lose sight of why linux-next exists. We want to
>>>> encourage code to appear there as a sounding board before it goes
>>>> mainline, so we can fix things and not pollute mainline git history
>>>> with those trivialities.
>>>>
>>>> If you've decided to internalize linux-next as part of your CI, then
>>>> great, but do note that does not elevate linux-next to some pristine
>>>> status for the world at large. That only means you have to watch more
>>>> closely what is going on.
>>>>
>>>> If you want to declare linux-next unbreakable -- well that would scare
>>>> away others to get the multi-arch or multi-config coverage that they may
>>>> not be able to do themselves. We are not going to do that.
>>>>
>>>> I have (hopefully) fixed the "bad commit" in v2 -- as part of the
>>>> implicit linux-next rule "you broke it, you better fix it ASAP".
>>>>
>>>> But "bad" and "miserable" can be things that might scare people off of
>>>> making use of linux-next for what it is meant to be for. And I am not
>>>> OK with that.
>>>
>>> They would need to use much stronger language to scare me off. That said,
>>> what on earth is the point of running tests if they do not from time to
>>> time find bugs? ;-)
>>
>> For me, part of the QA process is statically analyzing linux-next to
>> catch bugs before they land in linux. I think other testing is equally
>> worth while as catching bugs early saves time and money.
>
> All kidding aside, the fact that this appeared in -next was due to a
> mistake on my part, namely failing to push the changes before starting
> the test. Please accept my apologies, and I will continue to do my
> best to avoid this sort of thing.
>
> Thanx, Paul
No problem. I'm glad we have tools to catch issues like this.
Colin
>
>> Colin
>>
>>>
>>>> Thanks,
>>>> Paul.
>>>> --
>>>>
>>>>>
>>>>>> Signed-off-by: Colin Ian King
>>>>>> ---
>>>>>> lib/cpumask.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/lib/cpumask.c b/lib/cpumask.c
>>>>>> index 34ecb3005941..cb8a3ef0e73e 100644
>>>>>> --- a/lib/cpumask.c
>>>>>> +++ b/lib/cpumask.c
>>>>>> @@ -185,7 +185,7 @@ int __ref cpulist_parse(const char *buf, struct
>>>>>> cpumask
>>>>>> *dstp)
>>>>>> {
>>>>>> int r;
>>>>>> char *cpulist, last_cpu[5]; /* NR_CPUS <= */
>>>>>> -size_t len = strlen(buf);
>>>>>> +size_t len = strlen(buf) + 1;
>>>>>> bool early = !slab_is_available();
>>>>>> if (!strcmp(buf, "all")) {
>>>>>
>>
Re: [PATCH v2 0/4] support for global CPU list abbreviations
On 10/11/2020 15:32, Paul E. McKenney wrote: > On Mon, Nov 09, 2020 at 11:07:21PM -0500, Paul Gortmaker wrote: >> RFC/v1 ---> v2: >> >> commit #1: >>leave one line stub behind for !SMP solving build failures. >>Reported by Randy Dunlap and various build bots. >> >> commit #4 >>manage to remember '\0' char in strlen from one line to the next. >>Reported by Colin King. shouldn't that be "Reported and fixed by Colin King"? ;-) >> >> Original description from v1/RFC below remains unchanged... > > Queued and this time kicking off testing that actually includes your > patches! ;-) > > Thanx, Paul > >> --- >> >> The basic objective here was to add support for "nohz_full=8-last" and/or >> "rcu_nocbs="4-last" -- essentially introduce "last" as a portable >> reference evaluated at boot/runtime for anything using a CPU list. >> >> The thinking behind this, is that people carve off a few early CPUs to >> support housekeeping tasks, and perhaps dedicate one to a busy I/O >> peripheral, and then the remaining pool of CPUs out to the end are a >> part of a commonly configured pool used for the real work the user >> cares about. >> >> Extend that logic out to a fleet of machines - some new, and some >> nearing EOL, and you've probably got a wide range of core counts to >> contend with - even though the early number of cores dedicated to the >> system overhead probably doesn't vary. >> >> This change would enable sysadmins to have a common bootarg across all >> such systems, and would also avoid any off-by-one fencepost errors that >> happen for users who might briefly forget that core counts start at >> zero. >> >> Looking around before starting, I noticed RCU already had a short-form >> abbreviation "all" -- but if we want to treat CPU lists in a uniform >> matter, then tokens shouldn't be implemented at a subsystem level and >> hence be subsystem specific; each with their own variations. >> >> So I moved "all" to global use - for boot args, and for cgroups. Then >> I added the inverse "none" and finally, the one I wanted -- "last". >> >> The use of "last" isn't a standalone word like "all" or "none". It will >> be a part of a complete range specification, possibly with CSV separate >> ranges, and possibly specified multiple times. So I had to be a bit >> more careful with string matching - and hence un-inlined the parse >> function as commit #1 in this series. >> >> But it really is a generic support for "replace token ABC with known at >> boot value XYZ" - for example, it would be trivial to extend support to >> add "half" as a dynamic token to be replaced with 1/2 the core count, >> even though I wouldn't suggest that has a use case like "last" does. >> >> I tested the string matching with a bunch of intentionally badly crafted >> strings in a user-space harness, and tested bootarg use with nohz_full >> and rcu_nocbs, and also the post-boot cgroup use case as per below: >> >>root@hackbox:/sys/fs/cgroup/cpuset# mkdir foo >>root@hackbox:/sys/fs/cgroup/cpuset# cd foo >>root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus >> >>root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo 10-last > cpuset.cpus >>root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus >>10-15 >>root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo all > cpuset.cpus >>root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus >>0-15 >>root@hackbox:/sys/fs/cgroup/cpuset/foo# /bin/echo none > cpuset.cpus >>root@hackbox:/sys/fs/cgroup/cpuset/foo# cat cpuset.cpus >> >>root@hackbox:/sys/fs/cgroup/cpuset/foo# >> >> This was on a 16 core machine with CONFIG_NR_CPUS=16 in .config file. >> >> Note that the two use cases (boot and runtime) are why you see "early" >> parameter in the code - I entertained just sticking the string copy on >> the stack vs. the early alloc dance, but this felt more correct/robust. >> The cgroup and modular code using cpulist_parse() are runtime cases. >> >> --- >> >> Cc: Frederic Weisbecker >> Cc: "Paul E. McKenney" >> Cc: Josh Triplett >> Cc: Thomas Gleixner >> Cc: Ingo Molnar >> Cc: Li Zefan >> >> Paul Gortmaker (4): >> cpumask: un-inline cpulist_parse for SMP; prepare for ascii helpers >> cpumask: make "all" alias global and not just RCU >> cpumask: add a "none" alias to complement "all" >> cpumask: add "last" alias for cpu list specifications >> >> .../admin-guide/kernel-parameters.rst | 20 +++ >> .../admin-guide/kernel-parameters.txt | 4 +- >> include/linux/cpumask.h | 8 ++ >> kernel/rcu/tree_plugin.h | 13 +- >> lib/cpumask.c | 132 ++ >> 5 files changed, 165 insertions(+), 12 deletions(-) >> >> -- >> 2.25.1 >>
Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char
On 10/11/2020 15:24, Paul E. McKenney wrote:
> On Mon, Nov 09, 2020 at 11:57:15PM -0500, Paul Gortmaker wrote:
>>
>>
>> On 2020-11-09 8:07 p.m., Qian Cai wrote:
>>> On Mon, 2020-11-09 at 13:04 +, Colin King wrote:
>>>> From: Colin Ian King
>>>>
>>>> Currently the allocation of cpulist is based on the length of buf but does
>>>> not include the addition end of string '\0' terminator. Static analysis is
>>>> reporting this as a potential out-of-bounds access on cpulist. Fix this by
>>>> allocating enough space for the additional '\0' terminator.
>>>>
>>>> Addresses-Coverity: ("Out-of-bounds access")
>>>> Fixes: 65987e67f7ff ("cpumask: add "last" alias for cpu list
>>>> specifications")
>>>
>>> Yeah, this bad commit also introduced KASAN errors everywhere and then will
>>> disable lockdep that makes our linux-next CI miserable. Confirmed that this
>>> patch will fix it.
>>
>> I appreciate the reports reminding me why I hate touching string handling.
>>
>> But let us not lose sight of why linux-next exists. We want to
>> encourage code to appear there as a sounding board before it goes
>> mainline, so we can fix things and not pollute mainline git history
>> with those trivialities.
>>
>> If you've decided to internalize linux-next as part of your CI, then
>> great, but do note that does not elevate linux-next to some pristine
>> status for the world at large. That only means you have to watch more
>> closely what is going on.
>>
>> If you want to declare linux-next unbreakable -- well that would scare
>> away others to get the multi-arch or multi-config coverage that they may
>> not be able to do themselves. We are not going to do that.
>>
>> I have (hopefully) fixed the "bad commit" in v2 -- as part of the
>> implicit linux-next rule "you broke it, you better fix it ASAP".
>>
>> But "bad" and "miserable" can be things that might scare people off of
>> making use of linux-next for what it is meant to be for. And I am not
>> OK with that.
>
> They would need to use much stronger language to scare me off. That said,
> what on earth is the point of running tests if they do not from time to
> time find bugs? ;-)
>
> Thanx, Paul
For me, part of the QA process is statically analyzing linux-next to
catch bugs before they land in linux. I think other testing is equally
worth while as catching bugs early saves time and money.
Colin
>
>> Thanks,
>> Paul.
>> --
>>
>>>
>>>> Signed-off-by: Colin Ian King
>>>> ---
>>>> lib/cpumask.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/cpumask.c b/lib/cpumask.c
>>>> index 34ecb3005941..cb8a3ef0e73e 100644
>>>> --- a/lib/cpumask.c
>>>> +++ b/lib/cpumask.c
>>>> @@ -185,7 +185,7 @@ int __ref cpulist_parse(const char *buf, struct cpumask
>>>> *dstp)
>>>> {
>>>>int r;
>>>>char *cpulist, last_cpu[5]; /* NR_CPUS <= */
>>>> - size_t len = strlen(buf);
>>>> + size_t len = strlen(buf) + 1;
>>>>bool early = !slab_is_available();
>>>>if (!strcmp(buf, "all")) {
>>>
Re: net: dsa: hellcreek: Add support for hardware timestamping
On 09/11/2020 13:59, Kurt Kanzenbach wrote: > Hi Colin, > > On Mon Nov 09 2020, Colin Ian King wrote: >> Hi >> >> Static analysis on linux-next with Coverity has detected a potential >> null pointer dereference issue on the following commit: >> >> commit f0d4ba9eff75a79fccb7793f4d9f12303d458603 >> Author: Kamil Alkhouri >> Date: Tue Nov 3 08:10:58 2020 +0100 >> >> net: dsa: hellcreek: Add support for hardware timestamping >> >> The analysis is as follows: >> >> 323/* Get nanoseconds from ptp packet */ >> 324type = SKB_PTP_TYPE(skb); >> >>4. returned_null: ptp_parse_header returns NULL (checked 10 out of 12 >> times). >>5. var_assigned: Assigning: hdr = NULL return value from >> ptp_parse_header. >> >> 325hdr = ptp_parse_header(skb, type); >> >>Dereference null return value (NULL_RETURNS) >>6. dereference: Dereferencing a pointer that might be NULL hdr when >> calling hellcreek_get_reserved_field. >> >> 326ns = hellcreek_get_reserved_field(hdr); >> 327hellcreek_clear_reserved_field(hdr); >> >> This issue can only occur if the type & PTP_CLASS_PMASK is not one of >> PTP_CLASS_IPV4, PTP_CLASS_IPV6 or PTP_CLASS_L2. I'm not sure if this is >> a possibility or not, but I'm assuming that it would be useful to >> perform the null check just in case, but I'm not sure how this affects >> the hw timestamping code in this function. > > I don't see how the null pointer dereference could happen. That's the > Rx path you showed above. > > The counter part code is: > > hellcreek_port_rxtstamp: > > /* Make sure the message is a PTP message that needs to be timestamped >* and the interaction with the HW timestamping is enabled. If not, stop >* here >*/ > hdr = hellcreek_should_tstamp(hellcreek, port, skb, type); > if (!hdr) > return false; > > SKB_PTP_TYPE(skb) = type; > > Here the type is stored and hellcreek_should_tstamp() also calls > ptp_parse_header() internally. Only when ptp_parse_header() didn't > return NULL the first time the timestamping continues. It should be > safe. > > Also the error handling would be interesting at that point. What should > happen if the header is null then? Returning an invalid timestamp? > Ignore it? > > Hm. I think we have to make sure that it is a valid ptp packet before > reaching this code and that's what we've implemented. So, I guess it's > OK. OK - thanks, I'll mark this as a false positive. > > Thanks, > Kurt >
re: net: dsa: hellcreek: Add support for hardware timestamping
Hi Static analysis on linux-next with Coverity has detected a potential null pointer dereference issue on the following commit: commit f0d4ba9eff75a79fccb7793f4d9f12303d458603 Author: Kamil Alkhouri Date: Tue Nov 3 08:10:58 2020 +0100 net: dsa: hellcreek: Add support for hardware timestamping The analysis is as follows: 323/* Get nanoseconds from ptp packet */ 324type = SKB_PTP_TYPE(skb); 4. returned_null: ptp_parse_header returns NULL (checked 10 out of 12 times). 5. var_assigned: Assigning: hdr = NULL return value from ptp_parse_header. 325hdr = ptp_parse_header(skb, type); Dereference null return value (NULL_RETURNS) 6. dereference: Dereferencing a pointer that might be NULL hdr when calling hellcreek_get_reserved_field. 326ns = hellcreek_get_reserved_field(hdr); 327hellcreek_clear_reserved_field(hdr); This issue can only occur if the type & PTP_CLASS_PMASK is not one of PTP_CLASS_IPV4, PTP_CLASS_IPV6 or PTP_CLASS_L2. I'm not sure if this is a possibility or not, but I'm assuming that it would be useful to perform the null check just in case, but I'm not sure how this affects the hw timestamping code in this function. Colin
re: scsi: ufs: Try to save power mode change and UIC cmd completion timeout
Hi
Static analysis with Coverity on linux-next has detected a potential
null pointer deference issue with commit:
commit 0f52fcb99ea2738a0a0f28e12cf4dd427069dd2a
Author: Can Guo
Date: Mon Nov 2 22:24:40 2020 -0800
scsi: ufs: Try to save power mode change and UIC cmd completion timeout
The analysis is as follows:
4925 static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32
intr_status)
4926 {
4927irqreturn_t retval = IRQ_NONE;
4928
1. Condition intr_status & 1024, taking true branch.
2. Condition hba->active_uic_cmd, taking false branch.
3. var_compare_op: Comparing hba->active_uic_cmd to null implies
that hba->active_uic_cmd might be null.
4929if ((intr_status & UIC_COMMAND_COMPL) && hba->active_uic_cmd) {
4930hba->active_uic_cmd->argument2 |=
4931ufshcd_get_uic_cmd_result(hba);
4932hba->active_uic_cmd->argument3 =
4933ufshcd_get_dme_attr_val(hba);
4934if (!hba->uic_async_done)
4935hba->active_uic_cmd->cmd_active = 0;
4936complete(>active_uic_cmd->done);
4937retval = IRQ_HANDLED;
4938}
4939
4. Condition intr_status & (112U /* (0x40 | 0x20) | 0x10 */), taking
true branch.
5. Condition hba->uic_async_done, taking true branch.
4940if ((intr_status & UFSHCD_UIC_PWR_MASK) &&
hba->uic_async_done) {
Dereference after null check (FORWARD_NULL)
6. var_deref_op: Dereferencing null pointer hba->active_uic_cmd.
4941hba->active_uic_cmd->cmd_active = 0;
4942complete(hba->uic_async_done);
4943retval = IRQ_HANDLED;
4944}
4945
Line 4929 checks to see if hba->active_uic_cmd is null, so there is a
potential it may be null. However, on line 4941 hba->active_uic_cmd is
being dereferenced without a null check, so Coverity has flagged this is
a potential null pointer dereference issue.
If it is null, then cmd_active shouldn't be assigned, but I'm unsure if
this is a false positive warning and/or what the ramifications of not
seeting cmd_active to zero is if hba->active_uic_cmd is null.
Colin
Re: [PATCH][next] hwmon: corsair-psu: fix unintentional sign extension issue
On 05/11/2020 12:32, Wilken Gottwalt wrote:
> On Thu, 5 Nov 2020 11:50:19 +
> Colin King wrote:
>
>> From: Colin Ian King
>>
>> The shifting of the u8 integer data[3] by 24 bits to the left will
>> be promoted to a 32 bit signed int and then sign-extended to a
>> long. In the event that the top bit of data[3] is set then all
>> then all the upper 32 bits of a 64 bit long end up as also being
>> set because of the sign-extension. Fix this by casting data[3] to
>> a long before the shift.
>>
>> Addresses-Coverity: ("Unintended sign extension")
>> Fixes: ce15cd2cee8b ("hwmon: add Corsair PSU HID controller driver")
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/hwmon/corsair-psu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
>> index e92d0376e7ac..5d19a888231a 100644
>> --- a/drivers/hwmon/corsair-psu.c
>> +++ b/drivers/hwmon/corsair-psu.c
>> @@ -241,7 +241,7 @@ static int corsairpsu_get_value(struct corsairpsu_data
>> *priv, u8 cmd, u8
>> rail, l
>> * the LINEAR11 conversion are the watts values which are about 1200
>> for the strongest
>> psu
>> * supported (HX1200i)
>> */
>> -tmp = (data[3] << 24) + (data[2] << 16) + (data[1] << 8) + data[0];
>> +tmp = ((long)data[3] << 24) + (data[2] << 16) + (data[1] << 8) +
>> data[0];
>> switch (cmd) {
>> case PSU_CMD_IN_VOLTS:
>> case PSU_CMD_IN_AMPS:
>
> Yeah, this could happen if the uptime value in the micro-controller gets
> bigger
> than 68 years (in seconds), and it is the only value which actually uses more
> than 2 bytes for the representation. So what about architectures which are 32
> bit
> wide and where a long has 32 bits? I guess this simple cast is not enough.
For 32 bits (unsigned) the 4 u8 values in data represents ~136 years no
matter if we use a 32 or 64 bit long for tmp. The cast to long is
signed, so yes, that's ~68 years in seconds. So perhaps
corsairpsu_get_value() should be passing a unsigned long for the *val
arg and tmp should be unsigned long too?
>
> greetings,
> Wilken
>
re: spi: bcm2835: fix gpio cs level inversion
Hi,
Static analysis with coverity on today's linux-next has detected a
potential issue in bcm2835_spi_setup() in the following commit:
commit 5e31ba0c0543a04483b53151eb5b7413efece94c
Author: Martin Hundebøll
Date: Wed Oct 14 11:02:30 2020 +0200
spi: bcm2835: fix gpio cs level inversion
The analysis is as follows:
1191 static int bcm2835_spi_setup(struct spi_device *spi)
1192 {
1193struct spi_controller *ctlr = spi->controller;
1194struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
1195struct gpio_chip *chip;
1. var_decl: Declaring variable lflags without initializer.
... and later on ...
Uninitialized scalar variable (UNINIT)
9. uninit_use_in_call: Using uninitialized value lflags when calling
gpiochip_request_own_desc. [show details]
1262spi->cs_gpiod = gpiochip_request_own_desc(chip, 8 -
spi->chip_select,
1263 DRV_NAME,
1264 lflags,
1265 GPIOD_OUT_LOW);
The call to gpiochip_request_own_desc passes the uninitalized lflags
down to gpiod_configure_flags:
int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
3698unsigned long lflags, enum gpiod_flags dflags)
3699{
3700int ret;
3701
3702if (lflags & GPIO_ACTIVE_LOW)
3703set_bit(FLAG_ACTIVE_LOW, >flags);
3704
3705if (lflags & GPIO_OPEN_DRAIN)
3706set_bit(FLAG_OPEN_DRAIN, >flags);
so this looks like lflags needs to be initialized with something
legitimate, probably zero?
Colin
Re: [PATCH] ASoC: qcom: sm8250: Fix array out of bounds access
On 28/10/2020 14:20, Srinivas Kandagatla wrote:
> Static analysis Coverity had detected a potential array out-of-bounds
> write issue due to the fact that MAX AFE port Id was set to 16 instead
> of using AFE_PORT_MAX macro.
>
> Fix this by properly using AFE_PORT_MAX macro.
>
> Fixes: aa2e2785545a ("ASoC: qcom: sm8250: add sound card qrb5165-rb5 support")
> Reported-by: Colin Ian King
> Signed-off-by: Srinivas Kandagatla
> ---
> sound/soc/qcom/sm8250.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/sound/soc/qcom/sm8250.c b/sound/soc/qcom/sm8250.c
> index 7d43de6d909f..52c40512102f 100644
> --- a/sound/soc/qcom/sm8250.c
> +++ b/sound/soc/qcom/sm8250.c
> @@ -13,12 +13,11 @@
>
> #define DRIVER_NAME "sm8250"
> #define MI2S_BCLK_RATE 1536000
> -#define MAX_SDW_STREAMS 16
>
> struct sm8250_snd_data {
> - bool stream_prepared[MAX_SDW_STREAMS];
> + bool stream_prepared[AFE_PORT_MAX];
> struct snd_soc_card *card;
> - struct sdw_stream_runtime *sruntime[MAX_SDW_STREAMS];
> + struct sdw_stream_runtime *sruntime[AFE_PORT_MAX];
> };
>
> static int sm8250_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
>
Thanks, looks good to me.
Reviewed-by: Colin Ian King
Re: PM / devfreq: map devfreq drivers to governor using name
On 28/10/2020 12:00, Colin Ian King wrote:
> Hi,
>
> Static analysis of linux-next with Coverity has found a potential null
> pointer dereference issue with the following commit:
>
> commit 1b5c1be2c88e8445a20fa1929e26c37e7ca8c926
> Author: Nishanth Menon
> Date: Mon Oct 29 15:01:45 2012 -0500
>
> PM / devfreq: map devfreq drivers to governor using name
>
>
> The analysis is as follows for devfreq_remove_governor in
> drivers/devfreq/devfreq.c
>
> 1312
>
> deref_ptr_in_call: Dereferencing pointer devfreq->governor.
>
> 1313if (!strncmp(devfreq->governor->name, governor->name,
> 1314 DEVFREQ_NAME_LEN)) {
> 1315/* we should have a devfreq governor! */
>
> Dereference before null check (REVERSE_INULL)
> check_after_deref: Null-checking devfreq->governor suggests that it may
> be null, but it has already been dereferenced on all paths leading to
> the check.
>
> 1316if (!devfreq->governor) {
> 1317dev_warn(dev, "%s: Governor %s NOT
> present\n",
> 1318 __func__, governor->name);
> 1319continue;
> 1320/* Fall through */
>
> So devfreq->governor->name is dereferencing devfreq->governor before a
> null check on devfreq->governor
>
> Colin
>
I forgot to mention, an identical issue also exists here:
1247list_for_each_entry(devfreq, _list, node) {
1248int ret = 0;
1249struct device *dev = devfreq->dev.parent;
1250
deref_ptr_in_call: Dereferencing pointer devfreq->governor.
1251if (!strncmp(devfreq->governor->name, governor->name,
1252 DEVFREQ_NAME_LEN)) {
1253/* The following should never occur */
Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking devfreq->governor suggests that it may
be null, but it has already been dereferenced on all paths leading to
the check.
1254if (devfreq->governor) {
1255dev_warn(dev,
1256 "%s: Governor %s already
present\n",
1257 __func__,
devfreq->governor->name);
re: PM / devfreq: map devfreq drivers to governor using name
Hi,
Static analysis of linux-next with Coverity has found a potential null
pointer dereference issue with the following commit:
commit 1b5c1be2c88e8445a20fa1929e26c37e7ca8c926
Author: Nishanth Menon
Date: Mon Oct 29 15:01:45 2012 -0500
PM / devfreq: map devfreq drivers to governor using name
The analysis is as follows for devfreq_remove_governor in
drivers/devfreq/devfreq.c
1312
deref_ptr_in_call: Dereferencing pointer devfreq->governor.
1313if (!strncmp(devfreq->governor->name, governor->name,
1314 DEVFREQ_NAME_LEN)) {
1315/* we should have a devfreq governor! */
Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking devfreq->governor suggests that it may
be null, but it has already been dereferenced on all paths leading to
the check.
1316if (!devfreq->governor) {
1317dev_warn(dev, "%s: Governor %s NOT
present\n",
1318 __func__, governor->name);
1319continue;
1320/* Fall through */
So devfreq->governor->name is dereferencing devfreq->governor before a
null check on devfreq->governor
Colin
re: ASoC: qcom: sm8250: add sound card qrb5165-rb5 support
Hi,
Static analysis on linux-next with Coverity had detected a potential
array out-of-bounds write issue in the following commit:
commit aa2e2785545aab21b6cb2e23f111ae0751cbcca7
Author: Srinivas Kandagatla
Date: Mon Oct 26 17:09:47 2020 +
ASoC: qcom: sm8250: add sound card qrb5165-rb5 support
The analysis is as follows:
139 static int sm8250_snd_hw_free(struct snd_pcm_substream *substream)
140 {
141struct snd_soc_pcm_runtime *rtd = substream->private_data;
142struct sm8250_snd_data *data =
snd_soc_card_get_drvdata(rtd->card);
143struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
144struct sdw_stream_runtime *sruntime =
data->sruntime[cpu_dai->id];
145
1. Switch case value 105.
146switch (cpu_dai->id) {
2. equality_cond: Jumping to case 105.
147case WSA_CODEC_DMA_RX_0:
148case WSA_CODEC_DMA_RX_1:
Out-of-bounds write (OVERRUN)
3. Condition sruntime, taking true branch.
4. Condition data->stream_prepared[cpu_dai->id], taking true branch.
149if (sruntime && data->stream_prepared[cpu_dai->id]) {
150sdw_disable_stream(sruntime);
151sdw_deprepare_stream(sruntime);
Out-of-bounds write (OVERRUN)
5. overrun-local: Overrunning array data->stream_prepared of 16 bytes
at byte offset 105 using index cpu_dai->id (which evaluates to 105).
152data->stream_prepared[cpu_dai->id] = false;
153}
154break;
155default:
156break;
157}
158
159return 0;
160 }
So cpu_dia->id is 105 in this case statement, and yet
data->steam_prepared is an array of 16 elements, so this looks suspect.
Colin
Re: [PATCH][next] afs: fix a dereference on pointer cell before cell is null checked
On 27/10/2020 11:05, David Howells wrote: > Colin King wrote: > >> @@ -606,7 +605,7 @@ void afs_unuse_cell(struct afs_net *net, struct afs_cell >> *cell, enum afs_cell_tr >> >> u = atomic_read(>ref); >> a = atomic_dec_return(>active); >> -trace_afs_cell(debug_id, u, a, reason); >> +trace_afs_cell(cell->debug_id, u, a, reason); > > It's probably better to read cell->debug_id before calling > atomic_dec_return(). > > I have a patch for this based on a report by Dan Carpenter, so no need to send > a revised patch. OK - thanks David > > Thanks, > David >
Re: [PATCH] stop_machine: Mark functions as notrace
On 21/10/2020 08:38, Zong Li wrote:
> Like the commit cb9d7fd51d9f ("watchdog: Mark watchdog touch functions
> as notrace"), some architectures assume that the stopped CPUs don't make
> function calls to traceable functions when they are in the stopped
> state. For example, it causes unexpected kernel crashed when switching
> tracer on RISC-V.
>
> The following patches added calls to these two functions, fix it by
> adding the notrace annotations.
>
> Fixes: 4ecf0a43e729 ("processor: get rid of cpu_relax_yield")
> Fixes: 366237e7b083 ("stop_machine: Provide RCU quiescent state in
> multi_cpu_stop()")
>
> Signed-off-by: Zong Li
> ---
> kernel/rcu/tree.c | 2 +-
> kernel/stop_machine.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 06895ef85d69..2a52f42f64b6 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -409,7 +409,7 @@ bool rcu_eqs_special_set(int cpu)
> *
> * The caller must have disabled interrupts and must not be idle.
> */
> -void rcu_momentary_dyntick_idle(void)
> +notrace void rcu_momentary_dyntick_idle(void)
> {
> int special;
>
> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> index 865bb0228ab6..890b79cf0e7c 100644
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -178,7 +178,7 @@ static void ack_state(struct multi_stop_data *msdata)
> set_state(msdata, msdata->state + 1);
> }
>
> -void __weak stop_machine_yield(const struct cpumask *cpumask)
> +notrace void __weak stop_machine_yield(const struct cpumask *cpumask)
> {
> cpu_relax();
> }
>
Apologies for taking so long to reply, I needed to test this on several
devices.
This not only fixes the ftrace issue I see on RISC-V but also a ftrace
hang issue on ARM64 in 5.8 too.
Tested-by: Colin Ian King
Many thanks!
Re: default cpufreq gov, was: [PATCH] sched/fair: check for idle core
On 22/10/2020 15:52, Mel Gorman wrote: > On Thu, Oct 22, 2020 at 02:29:49PM +0200, Peter Zijlstra wrote: >> On Thu, Oct 22, 2020 at 02:19:29PM +0200, Rafael J. Wysocki wrote: However I do want to retire ondemand, conservative and also very much intel_pstate/active mode. >>> >>> I agree in general, but IMO it would not be prudent to do that without >>> making >>> schedutil provide the same level of performance in all of the relevant use >>> cases. >> >> Agreed; I though to have understood we were there already. > > AFAIK, not quite (added Giovanni as he has been paying more attention). > Schedutil has improved since it was merged but not to the extent where > it is a drop-in replacement. The standard it needs to meet is that > it is at least equivalent to powersave (in intel_pstate language) > or ondemand (acpi_cpufreq) and within a reasonable percentage of the > performance governor. Defaulting to performance is a) giving up and b) > the performance governor is not a universal win. There are some questions > currently on whether schedutil is good enough when HWP is not available. > There was some evidence (I don't have the data, Giovanni was looking into > it) that HWP was a requirement to make schedutil work well. That is a > hazard in itself because someone could test on the latest gen Intel CPU > and conclude everything is fine and miss that Intel-specific technology > is needed to make it work well while throwing everyone else under a bus. > Giovanni knows a lot more than I do about this, I could be wrong or > forgetting things. > > For distros, switching to schedutil by default would be nice because > frequency selection state would follow the task instead of being per-cpu > and we could stop worrying about different HWP implementations but it's > not at the point where the switch is advisable. I would expect hard data > before switching the default and still would strongly advise having a > period of time where we can fall back when someone inevitably finds a > new corner case or exception. ..and it would be really useful for distros to know when the hard data is available so that they can make an informed decision when to move to schedutil. > > For reference, SLUB had the same problem for years. It was switched > on by default in the kernel config but it was a long time before > SLUB was generally equivalent to SLAB in terms of performance. Block > multiqueue also had vaguely similar issues before the default changes > and a period of time before it was removed removed (example whinging mail > https://lore.kernel.org/lkml/20170803085115.r2jfz2lofy5sp...@techsingularity.net/) > It's schedutil's turn :P >
Re: [PATCH] vfio/fsl-mc: fix the return of the uninitialized variable ret
On 16/10/2020 10:26, Diana Craciun OSS wrote:
> On 10/15/2020 9:52 PM, Alex Williamson wrote:
>> On Thu, 15 Oct 2020 13:22:26 +0100
>> Colin King wrote:
>>
>>> From: Colin Ian King
>>>
>>> Currently the success path in function vfio_fsl_mc_reflck_attach is
>>> returning an uninitialized value in variable ret. Fix this by setting
>>> this to zero to indicate success.
>>>
>>> Addresses-Coverity: ("Uninitialized scalar variable")
>>> Fixes: f2ba7e8c947b ("vfio/fsl-mc: Added lock support in preparation
>>> for interrupt handling")
>>> Signed-off-by: Colin Ian King
>>> ---
>>> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> index 80fc7f4ed343..42a5decb78d1 100644
>>> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
>>> @@ -84,6 +84,7 @@ static int vfio_fsl_mc_reflck_attach(struct
>>> vfio_fsl_mc_device *vdev)
>>> vfio_fsl_mc_reflck_get(cont_vdev->reflck);
>>> vdev->reflck = cont_vdev->reflck;
>>> vfio_device_put(device);
>>> + ret = 0;
>>> }
>>> unlock:
>>
>> Looks correct to me, unless Diana would rather set the initial value to
>> zero instead. Thanks,
>>
>> Alex
>>
>
>
> I prefer to initialize the variable to 0 when it's defined. I'll send a
> patch for it.
Yep, works for me.
>
> Thanks,
> Diana
re: vfio/fsl-mc: trigger an interrupt via eventfd
Hi,
Static analysis with Coverity on linux-next today has detected an issue
in the following commit:
commit cc0ee20bd96971c10eba9a83ecf1c0733078a083
Author: Diana Craciun
Date: Mon Oct 5 20:36:52 2020 +0300
vfio/fsl-mc: trigger an interrupt via eventfd
The analysis is as follows:
106 static int vfio_fsl_mc_set_irq_trigger(struct vfio_fsl_mc_device *vdev,
107 unsigned int index, unsigned
int start,
108 unsigned int count, u32 flags,
109 void *data)
110 {
111struct fsl_mc_device *mc_dev = vdev->mc_dev;
112int ret, hwirq;
113struct vfio_fsl_mc_irq *irq;
114struct device *cont_dev = fsl_mc_cont_dev(_dev->dev);
115struct fsl_mc_device *mc_cont = to_fsl_mc_device(cont_dev);
116
cond_const: Condition count != 1U, taking false branch. Now the value of
count is equal to 1.
117if (start != 0 || count != 1)
118return -EINVAL;
119
120mutex_lock(>reflck->lock);
121ret = fsl_mc_populate_irq_pool(mc_cont,
122FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS);
123if (ret)
124goto unlock;
125
126ret = vfio_fsl_mc_irqs_allocate(vdev);
127if (ret)
128goto unlock;
129mutex_unlock(>reflck->lock);
130
const: At condition count, the value of count must be equal to 1.
dead_error_condition: The condition !count cannot be true.
Logically dead code (DEADCODE)
dead_error_line: Execution cannot reach the expression flags & 1U inside
this statement: if (!count && flags & 1U)
131if (!count && (flags & VFIO_IRQ_SET_DATA_NONE))
132return vfio_set_trigger(vdev, index, -1);
At line 131, count is 1 because of the check and return on lines
117-118. !count is 0, and so 0 && (flags & VFIO_IRQ_SET_DATA_NONE) is
always false, so the vfio_set_trigger is never called. I suspect that
was not the intention.
Colin
Re: [PATCH] ima: Fix sizeof mismatches
On 13/10/2020 17:17, Mimi Zohar wrote:
> On Mon, 2020-10-12 at 19:10 +0100, Colin Ian King wrote:
>> On 12/10/2020 19:06, Joe Perches wrote:
>>> On Mon, 2020-10-12 at 13:51 -0400, Mimi Zohar wrote:
>>>> On Wed, 2020-10-07 at 11:27 -0700, Joe Perches wrote:
>>>>> On Wed, 2020-10-07 at 12:02 +0100, Colin King wrote:
>>>>>> An incorrect sizeof is being used, sizeof(*fields) is not correct,
>>>>>> it should be sizeof(**fields). This is not causing a problem since
>>>>>> the size of these is the same. Fix this in the kmalloc_array and
>>>>>> memcpy calls.
>>>>> []
>>>>>> diff --git a/security/integrity/ima/ima_template.c
>>>>>> b/security/integrity/ima/ima_template.c
>>>>> []
>>>>>> @@ -216,11 +216,11 @@ int template_desc_init_fields(const char
>>>>>> *template_fmt,
>>>>>> }
>>>>>>
>>>>>> if (fields && num_fields) {
>>>>>> -*fields = kmalloc_array(i, sizeof(*fields), GFP_KERNEL);
>>>>>> +*fields = kmalloc_array(i, sizeof(**fields),
>>>>>> GFP_KERNEL);
>>>>>> if (*fields == NULL)
>>>>>> return -ENOMEM;
>>>>>>
>>>>>> -memcpy(*fields, found_fields, i * sizeof(*fields));
>>>>>> +memcpy(*fields, found_fields, i * sizeof(**fields));
>>>>>
>>>>> Maybe use kmemdup instead.
>>>>>
>>>>> if (fields && num_fields) {
>>>>> *fields = kmemdup(found_fields, i * sizeof(**fields),
>>>>> GFP_KERNEL);
>>>>> etc...
>>>>>
>>>>
>>>> Thanks, Joe. Since this patch will be backported, perhaps it would be
>>>> better to leave this as a bug fix and upstream other changes
>>>> independently.
>>>
>>> IMO:
>>>
>>> This patch doesn't need need backporting as it doesn't
>>> actually fix anything other than a style defect.
>>>
>>> void * and void ** are the same size.
>>
>> indeed, same size, it's a semantic difference *and* a style fix :-)
>
> Colin, based on Joe's suggestion of using kmemdup and his opinion of
> not backporting this change, can I assume you'll address his comments
> and re-post v3?
Oops, I missed that email. Yep, I'll address that later today
Colin
>
> thanks,
>
> Mimi
>
Re: [PATCH] ima: Fix sizeof mismatches
On 12/10/2020 19:06, Joe Perches wrote:
> On Mon, 2020-10-12 at 13:51 -0400, Mimi Zohar wrote:
>> On Wed, 2020-10-07 at 11:27 -0700, Joe Perches wrote:
>>> On Wed, 2020-10-07 at 12:02 +0100, Colin King wrote:
An incorrect sizeof is being used, sizeof(*fields) is not correct,
it should be sizeof(**fields). This is not causing a problem since
the size of these is the same. Fix this in the kmalloc_array and
memcpy calls.
>>> []
diff --git a/security/integrity/ima/ima_template.c
b/security/integrity/ima/ima_template.c
>>> []
@@ -216,11 +216,11 @@ int template_desc_init_fields(const char
*template_fmt,
}
if (fields && num_fields) {
- *fields = kmalloc_array(i, sizeof(*fields), GFP_KERNEL);
+ *fields = kmalloc_array(i, sizeof(**fields), GFP_KERNEL);
if (*fields == NULL)
return -ENOMEM;
- memcpy(*fields, found_fields, i * sizeof(*fields));
+ memcpy(*fields, found_fields, i * sizeof(**fields));
>>>
>>> Maybe use kmemdup instead.
>>>
>>> if (fields && num_fields) {
>>> *fields = kmemdup(found_fields, i * sizeof(**fields),
>>> GFP_KERNEL);
>>> etc...
>>>
>>
>> Thanks, Joe. Since this patch will be backported, perhaps it would be
>> better to leave this as a bug fix and upstream other changes
>> independently.
>
> IMO:
>
> This patch doesn't need need backporting as it doesn't
> actually fix anything other than a style defect.
>
> void * and void ** are the same size.
indeed, same size, it's a semantic difference *and* a style fix :-)
Colin
>
> cheers, Joe
>
re: RDMA/rxe: Fix skb lifetime in rxe_rcv_mcast_pkt()
Hi, Static analysis with Coverity has detected a potential issue with the following commit: commit e7ec96fc7932f48a6d6cdd05bf82004a1a04285b Author: Bob Pearson Date: Thu Oct 8 15:36:52 2020 -0500 RDMA/rxe: Fix skb lifetime in rxe_rcv_mcast_pkt() The analysis is as follows: 16. Condition mce->qp_list.next != >qp_list, taking true branch. 269if (mce->qp_list.next != >qp_list) 17. returned_null: skb_clone returns NULL (checked 153 out of 178 times). 18. var_assigned: Assigning: per_qp_skb = NULL return value from skb_clone. 19. Falling through to end of if statement. 270per_qp_skb = skb_clone(skb, GFP_ATOMIC); 271else 272per_qp_skb = skb; 273 274per_qp_pkt = SKB_TO_PKT(per_qp_skb); 275per_qp_pkt->qp = qp; 276rxe_add_ref(qp); Dereference null return value (NULL_RETURNS) 20. dereference: Dereferencing a pointer that might be NULL per_qp_skb when calling rxe_rcv_pkt. 277rxe_rcv_pkt(per_qp_pkt, per_qp_skb); 278} 279 It is possible for skb_clone to return NULL, so dereferencing the NULL return pointer per_qp_skb is a potential issue. Colin
[tip: core/rcu] refperf: Avoid null pointer dereference when buf fails to allocate
The following commit has been merged into the core/rcu branch of tip:
Commit-ID: 58db5785b0d76be4582a32a7900acce88e691d36
Gitweb:
https://git.kernel.org/tip/58db5785b0d76be4582a32a7900acce88e691d36
Author:Colin Ian King
AuthorDate:Thu, 16 Jul 2020 15:38:56 +01:00
Committer: Paul E. McKenney
CommitterDate: Mon, 24 Aug 2020 18:45:35 -07:00
refperf: Avoid null pointer dereference when buf fails to allocate
Currently in the unlikely event that buf fails to be allocated it
is dereferenced a few times. Use the errexit flag to determine if
buf should be written to to avoid the null pointer dereferences.
Addresses-Coverity: ("Dereference after null check")
Fixes: f518f154ecef ("refperf: Dynamically allocate experiment-summary output
buffer")
Signed-off-by: Colin Ian King
Signed-off-by: Paul E. McKenney
---
kernel/rcu/refscale.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
index d9291f8..952595c 100644
--- a/kernel/rcu/refscale.c
+++ b/kernel/rcu/refscale.c
@@ -546,9 +546,11 @@ static int main_func(void *arg)
// Print the average of all experiments
SCALEOUT("END OF TEST. Calculating average duration per loop
(nanoseconds)...\n");
- buf[0] = 0;
- strcat(buf, "\n");
- strcat(buf, "Runs\tTime(ns)\n");
+ if (!errexit) {
+ buf[0] = 0;
+ strcat(buf, "\n");
+ strcat(buf, "Runs\tTime(ns)\n");
+ }
for (exp = 0; exp < nruns; exp++) {
u64 avg;
Re: [PATCH] scsi: qla2xxx: fix return of uninitialized value in rval
On 08/10/2020 19:41, Pavel Machek wrote:
> On Thu 2020-10-08 19:32:39, Colin King wrote:
>> From: Colin Ian King
>>
>> A previous change removed the initialization of rval and there is
>> now an error where an uninitialized rval is being returned on an
>> error return path. Fix this by returning -ENODEV.
>>
>> Addresses-Coverity: ("Uninitialized scalar variable")
>> Fixes: b994718760fa ("scsi: qla2xxx: Use constant when it is known")
>> Signed-off-by: Colin Ian King
>
> Acked-by: Pavel Machek (CIP)
>
> And sorry, I did patch against mainline, and this got added in -next
> in the meantime.
Ah, that explains it. No problem, Coverity is good at finding buglets
Colin
>
>> index ae47e0eb0311..1f9005125313 100644
>> --- a/drivers/scsi/qla2xxx/qla_nvme.c
>> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
>> @@ -561,7 +561,7 @@ static int qla_nvme_post_cmd(struct nvme_fc_local_port
>> *lport,
>> vha = fcport->vha;
>>
>> if (!(fcport->nvme_flag & NVME_FLAG_REGISTERED))
>> -return rval;
>> +return -ENODEV;
>>
>> if (test_bit(ABORT_ISP_ACTIVE, >dpc_flags) ||
>> (qpair && !qpair->fw_started) || fcport->deleted)
>> --
>> 2.27.0
>>
>
re: io_uring: process task work in io_uring_register()
Hi,
Static analysis with Coverity has detected a "dead-code" issue with the
following commit:
commit af9c1a44f8dee7a958e07977f24ba40e3c770987
Author: Jens Axboe
Date: Thu Sep 24 13:32:18 2020 -0600
io_uring: process task work in io_uring_register()
The analysis is as follows:
9513do {
9514ret =
wait_for_completion_interruptible(>ref_comp);
cond_const: Condition ret, taking false branch. Now the value of ret is
equal to 0.
9515if (!ret)
9516break;
9517if (io_run_task_work_sig() > 0)
9518continue;
9519} while (1);
9520
9521mutex_lock(>uring_lock);
9522
const: At condition ret, the value of ret must be equal to 0.
dead_error_condition: The condition ret cannot be true.
9523if (ret) {
Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement:
9524percpu_ref_resurrect(>refs);
9525ret = -EINTR;
9526goto out_quiesce;
9527}
9528}
9529
Colin
Re: tracing: Add support for dynamic strings to synthetic events
On 07/10/2020 14:30, Steven Rostedt wrote:
> On Wed, 7 Oct 2020 14:08:38 +0100
> Colin Ian King wrote:
>
>> Hi,
>>
>> Static analysis with Coverity has detected a duplicated condition in an
>> if statement in the following commit in source
>> kernel/trace/trace_events_synth.c
>>
>> commit bd82631d7ccdc894af2738e47abcba2cb6e7dea9
>> Author: Tom Zanussi
>> Date: Sun Oct 4 17:14:06 2020 -0500
>>
>> tracing: Add support for dynamic strings to synthetic events
>>
>> Analysis is as follows:
>>
>> 493for (i = 0; i < event->n_fields; i++) {
>>
>> Same on both sides (CONSTANT_EXPRESSION_RESULT)
>> pointless_expression: The expression event->fields[i]->is_dynamic &&
>> event->fields[i]->is_dynamic does not accomplish anything because it
>> evaluates to either of its identical operands, event->fields[i]->is_dynamic.
>>
>>Did you intend the operands to be different?
>>
>> 494if (event->fields[i]->is_dynamic &&
>> 495event->fields[i]->is_dynamic)
>
> Bah, I believe that was suppose to be:
>
> if (event->fields[i]->is_string &&
> event->fields[i]->is_dynamic)
>
> I'll go and fix that.
Ah, makes sense. Thanks!
>
> -- Steve
>
>> 496pos += snprintf(buf + pos, LEN_OR_ZERO,
>> 497", __get_str(%s)",
>> event->fields[i]->name);
>> 498else
>> 499pos += snprintf(buf + pos, LEN_OR_ZERO,
>> 500", REC->%s",
>> event->fields[i]->name);
>> 501}
>>
>> Colin
>
re: tracing: Add support for dynamic strings to synthetic events
Hi,
Static analysis with Coverity has detected a duplicated condition in an
if statement in the following commit in source
kernel/trace/trace_events_synth.c
commit bd82631d7ccdc894af2738e47abcba2cb6e7dea9
Author: Tom Zanussi
Date: Sun Oct 4 17:14:06 2020 -0500
tracing: Add support for dynamic strings to synthetic events
Analysis is as follows:
493for (i = 0; i < event->n_fields; i++) {
Same on both sides (CONSTANT_EXPRESSION_RESULT)
pointless_expression: The expression event->fields[i]->is_dynamic &&
event->fields[i]->is_dynamic does not accomplish anything because it
evaluates to either of its identical operands, event->fields[i]->is_dynamic.
Did you intend the operands to be different?
494if (event->fields[i]->is_dynamic &&
495event->fields[i]->is_dynamic)
496pos += snprintf(buf + pos, LEN_OR_ZERO,
497", __get_str(%s)",
event->fields[i]->name);
498else
499pos += snprintf(buf + pos, LEN_OR_ZERO,
500", REC->%s",
event->fields[i]->name);
501}
Colin
