Re: [RFC PATCH 11/11] driver/int3400_thermal: Fix prototype matching

Kees Cook
2022-04-20T02:55:26Z

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
>

--
Kees Cook