linux-kernel - Re: [PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <x3wogjf6vgpkisdhg3abzrx7v7zktmdnfmqeih5kosszmagqfs@oh3qxrgzkikf>
Date: Wed, 27 Aug 2025 18:24:26 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Qianfeng Rong <rongqianfeng@...o.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>, 
	Jacopo Mondi <jacopo@...ndi.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, 
	"open list:MT9V111 APTINA CAMERA SENSOR" <linux-media@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] media: i2c: mt9v111: fix incorrect type for ret

Hi Qianfeng

On Wed, Aug 27, 2025 at 11:41:26PM +0800, Qianfeng Rong wrote:
>
> 在 2025/8/27 20:58, Jacopo Mondi 写道:
> > [You don't often get email from jacopo.mondi@...asonboard.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > Hi Qianfeng
> >
> > On Wed, Aug 27, 2025 at 08:39:10PM +0800, Qianfeng Rong wrote:
> > > Change "ret" from unsigned int to int type in mt9v111_calc_frame_rate()
> > > to store negative error codes or zero returned by __mt9v111_hw_reset()
> > > and other functions.
> > >
> > > Storing the negative error codes in unsigned type, doesn't cause an issue
> > > at runtime but it's ugly as pants.
> > >
> > > No effect on runtime.
> > well, I'm not sure that's true.
> >
> > The code goes as
> >
> >          ret = __mt9v111_hw_reset(mt9v111);
> >          if (ret == -EINVAL)
> >                  ret = __mt9v111_sw_reset(mt9v111);
> >          if (ret)
> >                  return ret;
> >
> > And if ret is unsigned the condition ret == -EINVAL will never be met.
> >
> > I guess this actually fixes a bug, doesn't it ?
> > You can add:
> >
> > Cc: stable@...r.kernel.org
> > Fixes: aab7ed1c3927 ("media: i2c: Add driver for Aptina MT9V111")
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> >
> > Thanks
> >    j
>
> I have written a test program on the arm64 platform:
>
> unsigned int ret = -ENOMEM;
>
> if (ret == -ENOMEM)
>     pr_info("unsigned int ret(%u) == -ENOMEM\n", ret);
> else
>     pr_info("unsigned int ret(%u) != -ENOMEM\n", ret);
>
> Output log is: unsigned int ret(4294967284) == -ENOMEM

Indeed, I was very wrong and ignoring the C integer promotion rules.

If I read right the "6.3.1.8 Usual arithmetic conversions" section of
the C11 specs I found freely available online (ISO/IEC 9899:201x), in
particular:

if the operand that has unsigned integer type has rank greater or
equal to the rank of the type of the other operand, then the operand with
signed integer type is converted to the type of the operand with unsigned
integer type.

Indeed the above doesn't introduce any functional change (as the
'rank' of an unsigned int is the same as the one of an int [1])

I would anyway consider it at least a "logical" fix, as comparing an
unsigned int to a negative error code is misleading for the reader at
the least.

Thanks anyway for testing.

[1] Section "6.3.1.1"
The rank of any unsigned integer type shall equal the rank of the corresponding
signed integer type, if any.

>
> I suspect that -ENOMEM is forcibly converted to an unsigned type during the
> comparison, but I am not sure if this behavior is consistent across all
> platforms and compilers. Therefore, I agree that your suggestion and will
> submit the v2 version.
>
> Best regards,
> Qianfeng
>