| Message ID | 20220420004241.2093-12-joao@overdrivepizza.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Kernel FineIBT Support | expand |
On Tue, Apr 19, 2022 at 05:42:41PM -0700, joao@overdrivepizza.com wrote: > From: Joao Moreira <joao@overdrivepizza.com> > > The function attr_dev_show directly invokes functions from drivers > expecting an specific prototype. The driver for int3400_thermal > implements the given show function using a different prototype than what > is expected. This violates the prototype-based fine-grained CFI policy. > > Make the function prototype compliant and cast the respective assignement > so it can be properly user together with fine-grained CFI. Does this trip on regular CFI? See below, but this all looks correct to me in the original code. > (FWIIW, there should be a less ugly patch for this, but I don't know > enough about the touched source code). > > Signed-off-by: Joao Moreira <joao@overdrivepizza.com> > --- > .../thermal/intel/int340x_thermal/int3400_thermal.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > index 4954800b9850..4bd95a2016b7 100644 > --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > @@ -311,12 +311,13 @@ static int int3400_thermal_get_uuids(struct int3400_thermal_priv *priv) > return result; > } > > -static ssize_t odvp_show(struct kobject *kobj, struct kobj_attribute *attr, > +static ssize_t odvp_show(struct device *kobj, struct device_attribute *attr, > char *buf) > { > + struct kobj_attribute *kattr = (struct kobj_attribute *) attr; > struct odvp_attr *odvp_attr; > > - odvp_attr = container_of(attr, struct odvp_attr, attr); > + odvp_attr = container_of(kattr, struct odvp_attr, attr); > > return sprintf(buf, "%d\n", odvp_attr->priv->odvp[odvp_attr->odvp]); > } > @@ -388,7 +389,10 @@ static int evaluate_odvp(struct int3400_thermal_priv *priv) > goto out_err; > } > odvp->attr.attr.mode = 0444; Eww, this function has a masked "odvp" variable here. One should be likely renamed. But anyway, odvp is: struct odvp_attr { int odvp; struct int3400_thermal_priv *priv; struct kobj_attribute attr; }; The original code looks correct to me (besides the masked variable name). kobj_attribute is part of odvp, the odvp_show callback has the correct prototype, and performs the correct container_of() to get odvp_attr. Where/why is the mismatch happening? -Kees > - odvp->attr.show = odvp_show; > + odvp->attr.show = (ssize_t (*) > + (struct kobject *, > + struct kobj_attribute *, > + char *)) odvp_show; > odvp->attr.store = NULL; > ret = sysfs_create_file(&priv->pdev->dev.kobj, > &odvp->attr.attr); > -- > 2.35.1 >
> Where/why is the mismatch happening?
Mismatch happens in dev_attr_show from drivers/base/core.c. There,
kobject * is cast to device * before the call, probably because attr is
also cast to device_attribute, which may have a mismatching hook
prototype, I guess.
I haven't tried it with any other CFI scheme other than my own
implementation and I did not run this on GDB or anything... I'm just
reporting based on the violation that FineIBT logged and in the fact
that this patch fixed it, so I'm unsure if there is anything buried
here.
On Wed, Apr 20, 2022 at 03:28:20PM -0700, Joao Moreira wrote: > > Where/why is the mismatch happening? > > Mismatch happens in dev_attr_show from drivers/base/core.c. There, kobject * > is cast to device * before the call, probably because attr is also cast to > device_attribute, which may have a mismatching hook prototype, I guess. Ah-ha, thanks. I think this will fix it, but I haven't tested it: diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c index 4954800b9850..d97f496bab9b 100644 --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c @@ -68,7 +68,7 @@ static int evaluate_odvp(struct int3400_thermal_priv *priv); struct odvp_attr { int odvp; struct int3400_thermal_priv *priv; - struct kobj_attribute attr; + struct device_attribute attr; }; static ssize_t data_vault_read(struct file *file, struct kobject *kobj, @@ -311,7 +311,7 @@ static int int3400_thermal_get_uuids(struct int3400_thermal_priv *priv) return result; } -static ssize_t odvp_show(struct kobject *kobj, struct kobj_attribute *attr, +static ssize_t odvp_show(struct device *dev, struct device_attribute *attr, char *buf) { struct odvp_attr *odvp_attr;
> > Ah-ha, thanks. I think this will fix it, but I haven't tested it: I tried that, and IIRC, there was an error or warning in the assignment that happens a bit further in the file. My bad for not having it all properly tracked.
On Wed, Apr 20, 2022 at 04:12:26PM -0700, Joao Moreira wrote: > > > > Ah-ha, thanks. I think this will fix it, but I haven't tested it: > > I tried that, and IIRC, there was an error or warning in the assignment that > happens a bit further in the file. My bad for not having it all properly > tracked. This builds for me without warnings, but maybe there is some weird config I'm missing.
On 2022-04-20 16:25, Kees Cook wrote: > On Wed, Apr 20, 2022 at 04:12:26PM -0700, Joao Moreira wrote: >> > >> > Ah-ha, thanks. I think this will fix it, but I haven't tested it: >> >> I tried that, and IIRC, there was an error or warning in the >> assignment that >> happens a bit further in the file. My bad for not having it all >> properly >> tracked. > > This builds for me without warnings, but maybe there is some weird > config I'm missing. Nah, I was just confused. This works fine and fixes the violation (confirmed after compiling and booting it).
diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c index 4954800b9850..4bd95a2016b7 100644 --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c @@ -311,12 +311,13 @@ static int int3400_thermal_get_uuids(struct int3400_thermal_priv *priv) return result; } -static ssize_t odvp_show(struct kobject *kobj, struct kobj_attribute *attr, +static ssize_t odvp_show(struct device *kobj, struct device_attribute *attr, char *buf) { + struct kobj_attribute *kattr = (struct kobj_attribute *) attr; struct odvp_attr *odvp_attr; - odvp_attr = container_of(attr, struct odvp_attr, attr); + odvp_attr = container_of(kattr, struct odvp_attr, attr); return sprintf(buf, "%d\n", odvp_attr->priv->odvp[odvp_attr->odvp]); } @@ -388,7 +389,10 @@ static int evaluate_odvp(struct int3400_thermal_priv *priv) goto out_err; } odvp->attr.attr.mode = 0444; - odvp->attr.show = odvp_show; + odvp->attr.show = (ssize_t (*) + (struct kobject *, + struct kobj_attribute *, + char *)) odvp_show; odvp->attr.store = NULL; ret = sysfs_create_file(&priv->pdev->dev.kobj, &odvp->attr.attr);