Hi Martin,
On 11/10/2023 02:35, Pawan Gupta wrote:
> On Tue, Oct 10, 2023 at 12:31:07PM +0200, Martin Tůma wrote:
>> On 06. 10. 23 12:08, Hans Verkuil wrote:
>>> Fix smatch warnings:
>>>
>>> drivers/media/pci/mgb4/mgb4_sysfs_out.c:118 video_source_store() warn: potential spectre issue 'mgbdev->vin' [r] (local cap)
>>> drivers/media/pci/mgb4/mgb4_sysfs_out.c:122 video_source_store() warn: possible spectre second half. 'loopin_new'>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>> CC: Martin Tuma <martin.tuma@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>> drivers/media/pci/mgb4/mgb4_sysfs_out.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/pci/mgb4/mgb4_sysfs_out.c b/drivers/media/pci/mgb4/mgb4_sysfs_out.c
>>> index 23a9aabf3915..9f6e81c57726 100644
>>> --- a/drivers/media/pci/mgb4/mgb4_sysfs_out.c
>>> +++ b/drivers/media/pci/mgb4/mgb4_sysfs_out.c
>>> @@ -8,6 +8,7 @@
>>> */
>>> #include <linux/device.h>
>>> +#include <linux/nospec.h>
>>> #include "mgb4_core.h"
>>> #include "mgb4_i2c.h"
>>> #include "mgb4_vout.h"
>>> @@ -114,8 +115,10 @@ static ssize_t video_source_store(struct device *dev,
>>> if (((config & 0xc) >> 2) < MGB4_VIN_DEVICES)
>>> loopin_old = mgbdev->vin[(config & 0xc) >> 2];
>>> - if (val < MGB4_VIN_DEVICES)
>>> + if (val < MGB4_VIN_DEVICES) {
>>> + val = array_index_nospec(val, MGB4_VIN_DEVICES);
>>> loopin_new = mgbdev->vin[val];
>>> + }
>>> if (loopin_old && loopin_cnt(loopin_old) == 1)
>>> mgb4_mask_reg(&mgbdev->video, loopin_old->config->regs.config,
>>> 0x2, 0x0);
>>
>> Hi,
>> I had investigated the warning when it appeared here on the mailing list,
>> but IMHO it is a false-positive, so I didn't react to it. MGB4_VIN_DEVICES
>> in the condition is not a check for array bounds but a check whether the
>> input source (val) is one of the inputs. Valid "val" values are <0,3> and
>> only if the value is <0,1> it is used as an array index for the input
>> devices (vin) array.
>
> I think when there are 2 branch mispredicts this is a valid problem.
> Below both branches can be trained with a val < 2. Then for a val > 3,
> both branches can mispredict:
>
> video_source_store(buf)
> {
> ...
> ret = kstrtoul(buf, 10, &val);
> if (ret)
> return ret;
> if (val > 3) <------------- predicted as not taken
> return -EINVAL;
> ...
>
> if (val < MGB4_VIN_DEVICES) <------------- predicted as taken
> loopin_new = mgbdev->vin[val];
>
> The fix LGTM, although it can also possibly be fixed by masking the
> index after the first mispredicted branch, like:
>
> ret = kstrtoul(buf, 10, &val);
> if (ret)
> return ret;
>
> + val = array_index_nospec(val, 4);
>
> provided, mgbdev->vin[2] and mgbdev->vin[3] can't load a secret.
Based on that input, and also because I want to shut up that warnings :-),
is it OK to merge? Can you proved and Acked-by or Reviewed-by?
Thanks!
Hans
