linux-kernel - RE: [PATCH v3 03/10] mtd: intel-dg: implement access functions

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] [day] [month] [year] [list]

Message-ID: <CY5PR11MB6366ACD36AF6AC5B967A57B1ED062@CY5PR11MB6366.namprd11.prod.outlook.com>
Date: Thu, 19 Dec 2024 10:39:37 +0000
From: "Usyskin, Alexander" <alexander.usyskin@...el.com>
To: "Vivi, Rodrigo" <rodrigo.vivi@...el.com>
CC: Miquel Raynal <miquel.raynal@...tlin.com>, Richard Weinberger
	<richard@....at>, Vignesh Raghavendra <vigneshr@...com>, "De Marchi, Lucas"
	<lucas.demarchi@...el.com>, Thomas Hellström
	<thomas.hellstrom@...ux.intel.com>, Maarten Lankhorst
	<maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
	Simona Vetter <simona@...ll.ch>, Jani Nikula <jani.nikula@...ux.intel.com>,
	Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>, Tvrtko Ursulin
	<tursulin@...ulin.net>, "Weil, Oren jer" <oren.jer.weil@...el.com>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Tomas Winkler
	<tomasw@...il.com>, Vitaly Lubart <lubvital@...il.com>
Subject: RE: [PATCH v3 03/10] mtd: intel-dg: implement access functions

> > +
> > +static ssize_t idg_nvm_rewrite_partial(struct intel_dg_nvm *nvm, loff_t to,
> > +				       loff_t offset, size_t len, const u32
> *newdata)
> > +{
> > +	u32 data = idg_nvm_read32(nvm, to);
> > +
> > +	if (idg_nvm_error(nvm))
> > +		return -EIO;
> > +
> > +	memcpy((u8 *)&data + offset, newdata, len);
> 
> I'm a bit concerned with the usage of len here without any check...
> 

This is an internal helper; we can rely on caller to do things right.

> > +
> > +	idg_nvm_write32(nvm, to, data);
> > +	if (idg_nvm_error(nvm))
> > +		return -EIO;
> > +
> > +	return len;
> > +}
> > +
> > +__maybe_unused
> > +static ssize_t idg_write(struct intel_dg_nvm *nvm, u8 region,
> > +			 loff_t to, size_t len, const unsigned char *buf)
> > +{
> > +	size_t i;
> > +	size_t len8;
> > +	size_t len4;
> > +	size_t to4;
> > +	size_t to_shift;
> > +	size_t len_s = len;
> > +	ssize_t ret;
> > +
> > +	idg_nvm_set_region_id(nvm, region);
> > +
> > +	to4 = ALIGN_DOWN(to, sizeof(u32));
> > +	to_shift = min(sizeof(u32) - ((size_t)to - to4), len);
> > +	if (to - to4) {
> 
> As well the 'to'...
> 
> I was even trying to review all 3 patches together, 3, 4, and 5,
> but there's too much indirection on those values and no checks
> in any place...
> 

These are callbacks for mtd framework. 
The mtd checks sanity of values before passing to callbacks, like:
https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/mtd/mtdcore.c#L1570

Should we double-check input here?


- - 
Thanks,
Sasha