Re: [PATCH 2/7] tpm: reorganize sysfs files - Updated patch

Kylene Jo Hall
2006-04-11T14:24:06Z

On Mon, 2006-04-10 at 14:46 -0700, Andrew Morton wrote:
> Kylene Jo Hall <kjhall@us.ibm.com> wrote:
> >
> > ssize_t tpm_show_pcrs(struct device *dev, struct device_attribute *attr,
> > char *buf)
> > {
> > - u8 data[READ_PCR_RESULT_SIZE];
> > - ssize_t len;
> > + u8 data[30];
> > + ssize_t rc;
> > int i, j, num_pcrs;
> > __be32 index;
> > char *str = buf;
> > @@ -150,29 +190,24 @@ ssize_t tpm_show_pcrs(struct device *dev
> > if (chip == NULL)
> > return -ENODEV;
> >
> > - memcpy(data, cap_pcr, sizeof(cap_pcr));
> > - if ((len = tpm_transmit(chip, data, sizeof(data)))
> > - < CAP_PCR_RESULT_SIZE) {
> > - dev_dbg(chip->dev, "A TPM error (%d) occurred "
> > - "attempting to determine the number of PCRS\n",
> > - be32_to_cpu(*((__be32 *) (data + 6))));
> > + memcpy(data, tpm_cap, sizeof(tpm_cap));
>
> I'd be a bit worried about potential for array overruns here. If someone
> later were to increase the size of tpm_cap[] we'll silently overrun data[].
>
> One approach would be to do:
>
> --- devel/drivers/char/tpm/tpm.c~tpm-reorganize-sysfs-files-fix 2006-04-10 14:43:16.000000000 -0700
> +++ devel-akpm/drivers/char/tpm/tpm.c 2006-04-10 14:45:19.000000000 -0700
> @@ -180,7 +180,7 @@ static const u8 pcrread[] = {
> ssize_t tpm_show_pcrs(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - u8 data[30];
> + u8 data[ARRAY_SIZE(tpm_cap)];
> ssize_t rc;
> int i, j, num_pcrs;
> __be32 index;
> @@ -296,7 +296,7 @@ static const u8 cap_version[] = {
> ssize_t tpm_show_caps(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - u8 data[30];
> + u8 data[max(ARRAY_SIZE(tpm_cap), ARRAY_SIZE(cap_version))];
> ssize_t rc;
> char *str = buf;
>
> _
>
>
> Does that look OK?

No this is not ok because in several of these cases the response to the
command is longer than tpm_cap thus the reason for the hardcoded size.
I can put in a max function though that compares the size of the
response and the tpm_cap. The read functions will make sure the
response does not overflow the buffer should that length ever change in
the future.

Thanks,
Kylie