linux-kernel - Re: [PATCH v1 4/7] rtc-rv8803: add tamper function to sysfs for rv8901

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: <spr5fq42lc4qyhn35hcruficop67trmbznxel4ndibtk3yldsd@7c6ufblhbugj>
Date: Sat, 11 Jan 2025 11:24:04 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Markus Burri <markus.burri@...com>
Cc: linux-kernel@...r.kernel.org, 
	Alexandre Belloni <alexandre.belloni@...tlin.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Marek Vasut <marex@...x.de>, 
	linux-rtc@...r.kernel.org, devicetree@...r.kernel.org, Manuel Traut <manuel.traut@...com>
Subject: Re: [PATCH v1 4/7] rtc-rv8803: add tamper function to sysfs for
 rv8901

On Fri, Jan 10, 2025 at 07:13:58AM +0100, Markus Burri wrote:
> +
> +static ssize_t enable_store(struct device *dev, struct device_attribute *attr, const char *buf,
> +			    size_t count)
> +{
> +	int ret;
> +	int i;
> +	unsigned long tmo;
> +	u8 reg;
> +	u8 reg_mask;
> +	struct rv8803_data *rv8803 = dev_get_drvdata(dev->parent);
> +	struct i2c_client *client = rv8803->client;
> +
> +	/* EVINxCPEN | EVINxEN */;
> +	const u8 reg_mask_evin_en = GENMASK(5, 3) | GENMASK(2, 0);
> +
> +	bool enable = (strstr(buf, "1") == buf) ? true : false;
> +
> +	guard(mutex)(&rv8803->flags_lock);


That's absolutely huge guard. Isn't this supposed to protect only flags?
Not all register writes?

> +
> +	/* check if event detection status match requested mode */
> +	ret = rv8803_read_reg(client, RX8901_EVIN_EN);
> +	if (ret < 0)
> +		return ret;

...

> +	/* re-enable interrupts */
> +	ret = rv8803_read_reg(client, RV8803_CTRL);
> +	if (ret < 0)
> +		return ret;
> +	ret = rv8803_write_reg(client, RV8803_CTRL, ret | RV8803_CTRL_EIE);
> +	if (ret < 0)
> +		return ret;
> +
> +	return offset;
> +}
> +
> +static DEVICE_ATTR_WO(enable);

You need to document the ABI.

Best regards,
Krzysztof