Re: [PATCH resend] mfd: mt6370: add bounds checking to regmap_read/write functions

From: Dan Carpenter <dan.carpenter@oracle.com>
To: ChiYuan Huang <u0084500@gmail.com>
Cc: Lee Jones <lee@kernel.org>, ChiYuan Huang <cy_huang@richtek.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	ChiaEn Wu <chiaen_wu@richtek.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	kernel-janitors@vger.kernel.org
Subject: Re: [PATCH resend] mfd: mt6370: add bounds checking to regmap_read/write functions
Date: Thu, 27 Oct 2022 16:59:28 +0300	[thread overview]
Message-ID: <Y1qOwO11z0Wvcz6O@kadam> (raw)
In-Reply-To: <CADiBU38JKzq9ngiaZWTse3eMdH_fTcUCdanea7uQAU7NxZrzZg@mail.gmail.com>

On Thu, Oct 27, 2022 at 09:59:46AM +0800, ChiYuan Huang wrote:
> ChiYuan Huang <u0084500@gmail.com> 於 2022年10月26日 週三 下午5:05寫道：
> >
> > Dan Carpenter <dan.carpenter@oracle.com> 於 2022年10月26日 週三 下午4:51寫道：
> > >
> > > On Wed, Oct 26, 2022 at 03:24:48PM +0800, ChiYuan Huang wrote:
> > > > 2) normal register access with negative length
> > > > Unable to handle kernel paging request at virtual address ffffffc009cefff2
> > > > pc : __memcpy+0x1dc/0x260
> > > > lr : _regmap_raw_write_impl+0x6d4/0x828
> > > > Call trace:
> > > >  __memcpy+0x1dc/0x260
> > > >  _regmap_raw_write+0xb4/0x130a
> > > >  regmap_raw_write+0x74/0xb0
> > > >
> > > >
> > > > After applying the patch, the first case is cleared.
> > > > But for the case 2, the root cause is not the mt6370_regmap_write() size
> > > > check. It's in __memcpy() before mt6370_regmap_write().
> > > >
> > > > I'm wondering 'is it reasonable to give the negative value as the size?'
> > > >
> > >
> > > Thanks for testing!
> > >
> > > I'm not sure I understand exactly which code you're talking about.
> > > Could you just create a diff with the check for negative just so I can
> > > understand where the issue is?  We can re-work it into a proper patch
> > > from there.
> > >
> > Here.
> > https://elixir.bootlin.com/linux/v6.1-rc2/source/drivers/base/regmap/regmap.c#L1860
> >
> > From my experiment, I try to access 0x00 reg for size (-1).
> > Testing code is like as below
> > regmap_raw_write(regmap, 0, &val, -1);
> >
> > That's why I think if the size check is needed, it may put into
> > regmap_raw_write() like as regmap_raw_read().
> >
> It seems c99 already  said size_t is an unsigned integer type.
> My experiment for (-1) size is not reasonable.
> (-1) means it will be converted as the UINT_MAX or ULONG_MAX.
> This will cause any unknown error like as memory violation or stack
> protection,...etc.
> 
> let's check whether the negative size is reasonable or not.
> If this case dost not exist, to keep the boundary check is enough.

I thought you were testing this from user space but it sounds like
you're doing a unit test?

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel