linux-hardening - Re: [RFC PATCH 11/11] driver/int3400

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: <202204191946.2843CF71@keescook>
Date:   Tue, 19 Apr 2022 19:55:19 -0700
From:   Kees Cook <keescook@...omium.org>
To:     joao@...rdrivepizza.com
Cc:     linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
        peterz@...radead.org, jpoimboe@...hat.com,
        andrew.cooper3@...rix.com, samitolvanen@...gle.com,
        mark.rutland@....com, hjl.tools@...il.com,
        alyssa.milburn@...ux.intel.com, ndesaulniers@...gle.com,
        gabriel.gomes@...ux.intel.com, rick.p.edgecombe@...el.com
Subject: Re: [RFC PATCH 11/11] driver/int3400_thermal: Fix prototype matching

On Tue, Apr 19, 2022 at 05:42:41PM -0700, joao@...rdrivepizza.com wrote:
> From: Joao Moreira <joao@...rdrivepizza.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@...rdrivepizza.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