On Tue, Apr 19, 2022 at 05:42:41PM -0700, [email protected] wrote:
> From: Joao Moreira <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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
>
--
Kees Cook
> 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;
--
Kees Cook
>
> 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.