linux-kernel - Re: [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper detection

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]
Date:   Thu, 8 Mar 2018 11:53:09 +0000
From:   Denis OSTERLAND <denis.osterland@...hl.com>
To:     "alexandre.belloni@...e-electrons.com" 
        <alexandre.belloni@...e-electrons.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mgr@...gutronix.de" <mgr@...gutronix.de>,
        "m.grzeschik@...gutronix.de" <m.grzeschik@...gutronix.de>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "a.zummo@...ertech.it" <a.zummo@...ertech.it>,
        "linux@...ck-us.net" <linux@...ck-us.net>,
        "jdelvare@...e.com" <jdelvare@...e.com>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: Re: [PATCH v3 3/4] rtc: isl1208: add support for isl1219 with tamper
 detection

Am Mittwoch, den 07.03.2018, 11:47 +0100 schrieb Alexandre Belloni:
> > > > +
> > > > +	tv64.tv_sec = rtc_tm_to_time64(&tm);
> > > Why not using an unsigned long long directly here? time64_t is not the
> > > correct type.
> > Do you mean timespec64 is not the correct type here?
> > Then yes, sould be time64_t.
> > If you mean time64_t is not the correct type here,
> > then can you give me some detail why there is no rtc_tm_to_u64,
> > or something like that?
> The rtc subsystem forbids negative times, the proper type should be
> unsigned.
I will add rtc_vaild_tm check.

Which sequence for time conversion would you expect?

time64_t secs = rtc_tm_to_time64(&tm);
BUG_ON(secs < 0);
return sprintf(buf, "%llu\n", (unsigned long long)secs);

or

return sprintf(buf, "%llu\n", (unsigned long long)rtc_tm_to_time64(&tm));
> 
> > 
> > sprintf(buf, "%lld\n", rtc_tm_to_time64(&tm)) seems correct to me.
> > By the way, is it needed to check for seconds < 0 and return error?
> Indeed, you shoud check the tm with rtc_valid_tm before calling
> rtc_tm_to_time64.
> 
> > 
> > > 
> > > > 
> > > > -	rc = sysfs_create_group(&client->dev.kobj, &isl1208_rtc_sysfs_files);
> > > > +	if (id->driver_data == TYPE_ISL1219) {
> > > > +		rc = i2c_smbus_write_byte_data(client, ISL1208_REG_09, 0x10);
> > > > +		if (rc < 0) {
> > > > +			dev_err(&client->dev, "could not enable tamper detection\n");
> > > > +			return rc;
> > > > +		}
> > > > +		isl1208->sysfs_files = &isl1219_rtc_sysfs_files;
> > > > +	} else {
> > > > +		isl1208->sysfs_files = &isl1208_rtc_sysfs_files;
> > > > +	}
> > > > +
> > > I don't think the whole isl1208 is necessary. You should probably use
> > > the .is_visible callback of isl1219_rtc_sysfs_files. This will make the
> > > changelog quite smaller.
> > > 
> > Well, I don´t know how to access i2c_device_id from kobject.
> > rtc_attr_is_visible shows how to convert kobject to device and rtc_device,
> > but how to do (id->driver_data == TYPE_ISL1219) here?
> I'd use i2c_set_clientdata/i2c_get_clientdata but I agree that then it
> is basically the same as having isl1208->sysfs_files.
> 
> but this makes me realize that the timestamp file doesn't end up at the
> correct location. What you do now is placing it under the i2c device
> while it should be placed under the rtc device (i.e. in
> /sys/class/rtc/rtcX/). This was a mistake made back in 2006.
> 
> I guess you'll have to add a new group instead of adding to the current
> one.
I guess I found a way to do it.

static struct attribute *isl1219_rtc_attrs[] = {
	&dev_attr_timestamp0.attr,
	NULL
};

in probe
	if (id->driver_data == TYPE_ISL1219) {
		sysfs_merge_group(&rtc->kobj, &isl1219_rtc_sysfs_files);

in remove
	struct rtc_device *rtc = i2c_get_clientdata(client);
	sysfs_unmerge_group(&rtc->kobj, &isl1219_rtc_sysfs_files);

As far as I got it, I can call unmerge even if group was not merged before.
If it works I don´t need struct isl1208 at all.
> 
Diehl AKO Stiftung & Co. KG, Pfannerstraße 75-83, 88239 Wangen im Allgäu
Bereichsvorstand: Dr.-Ing. Michael Siedentop (Sprecher), Josef Fellner (Mitglied)
Sitz der Gesellschaft: Wangen i.A. – Registergericht: Amtsgericht Ulm HRA 620609 – Persönlich haftende Gesellschafterin: Diehl Verwaltungs-Stiftung – Sitz: Nürnberg – Registergericht: Amtsgericht Nürnberg HRA 11756 –
Vorstand: Dr.-Ing. E.h. Thomas Diehl (†) (Vorsitzender), Herr Dipl.-Wirtsch.-Ing. Wolfgang Weggen (stellvertretender Vorsitzender), Dipl.-Kfm. Claus Günther, Dipl.-Kfm. Frank Gutzeit, Dr.-Ing. Heinrich Schunk, Dr.-Ing. Michael Siedentop , Dipl.-Kfm. Dr.-Ing. Martin Sommer, Dipl.-Ing. (FH) Rainer von Borstel, Vorsitzender des Aufsichtsrates: Dr. Klaus Maier
___________________________________________________________________________________________________
Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht. Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited.