All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ivan.khoronzhuk" <ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
To: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org,
	ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
	linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	mikew-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org,
	dmidecode-devel-qX2TKyscuCcdnm+yROfE0A@public.gmane.org,
	leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
	msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
	roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org
Subject: Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
Date: Tue, 21 Apr 2015 19:03:30 +0300	[thread overview]
Message-ID: <553674D2.10407@globallogic.com> (raw)
In-Reply-To: <20150421173613.6601da15-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org>

Jean,

On 21.04.15 18:36, Jean Delvare wrote:
> Hi again Ivan,
>
> On Mon, 20 Apr 2015 13:19:46 +0300, Ivan Khoronzhuk wrote:
>> +	bin_attr_DMI.size = dmi_len;
>> +	bin_attr_DMI.private = dmi_table;
>> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
>> +	if (!ret)
>> +		return 0;
> I just found that more work is needed here for the SMBIOS v3 entry
> point case. These entry points do not specify the exact length of the
> table, but only its maximum. The real world sample I have access to
> indeed specifies a maximum length of 6419 bytes, but the actual table
> only spans over 2373 bytes. It is properly terminated with a type 127
> DMI structure, so the kernel table parser ignores the garbage after it.
> The garbage is however exported to user-space above.
>
> I taught dmidecode to ignore the garbage, but there are two problem
> left here. First problem is a waste of memory. Minor issue I suppose,
> who cares about a few kilobytes these days.
>
> Second problem is a security problem. We are leaking the contents of
> physical memory to user-space. In my case it's filled with 0xffs so no
> big deal. But what if actual data happens to be stored there? It
> definitely shouldn't go to user-space.
>
> So dmi_len needs to be trimmed to the actual table size before the
> attribute above is created. I have an idea how this could be
> implemented easily, let me give it a try.
>
> Maybe we should trim the length for previous implementations, too.
> There is no reason to walk past a type 127 structure anyway, ever.
>

It can happen of-cause, I've also thought about that sometime ago,
but forget...).
I've sent the updated series already.
Let me know when your fix will be ready and I will re-base the
series if it has conflicts.

-- 
Regards,
Ivan Khoronzhuk

WARNING: multiple messages have this Message-ID (diff)
From: "Ivan.khoronzhuk" <ivan.khoronzhuk@globallogic.com>
To: Jean Delvare <jdelvare@suse.de>
Cc: linux-kernel@vger.kernel.org, matt.fleming@intel.com,
	ard.biesheuvel@linaro.org, grant.likely@linaro.org,
	linux-api@vger.kernel.org, linux-doc@vger.kernel.org,
	mikew@google.com, dmidecode-devel@nongnu.org,
	leif.lindholm@linaro.org, msalter@redhat.com,
	roy.franz@linaro.org
Subject: Re: [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables
Date: Tue, 21 Apr 2015 19:03:30 +0300	[thread overview]
Message-ID: <553674D2.10407@globallogic.com> (raw)
In-Reply-To: <20150421173613.6601da15@endymion.delvare>

Jean,

On 21.04.15 18:36, Jean Delvare wrote:
> Hi again Ivan,
>
> On Mon, 20 Apr 2015 13:19:46 +0300, Ivan Khoronzhuk wrote:
>> +	bin_attr_DMI.size = dmi_len;
>> +	bin_attr_DMI.private = dmi_table;
>> +	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
>> +	if (!ret)
>> +		return 0;
> I just found that more work is needed here for the SMBIOS v3 entry
> point case. These entry points do not specify the exact length of the
> table, but only its maximum. The real world sample I have access to
> indeed specifies a maximum length of 6419 bytes, but the actual table
> only spans over 2373 bytes. It is properly terminated with a type 127
> DMI structure, so the kernel table parser ignores the garbage after it.
> The garbage is however exported to user-space above.
>
> I taught dmidecode to ignore the garbage, but there are two problem
> left here. First problem is a waste of memory. Minor issue I suppose,
> who cares about a few kilobytes these days.
>
> Second problem is a security problem. We are leaking the contents of
> physical memory to user-space. In my case it's filled with 0xffs so no
> big deal. But what if actual data happens to be stored there? It
> definitely shouldn't go to user-space.
>
> So dmi_len needs to be trimmed to the actual table size before the
> attribute above is created. I have an idea how this could be
> implemented easily, let me give it a try.
>
> Maybe we should trim the length for previous implementations, too.
> There is no reason to walk past a type 127 structure anyway, ever.
>

It can happen of-cause, I've also thought about that sometime ago,
but forget...).
I've sent the updated series already.
Let me know when your fix will be ready and I will re-base the
series if it has conflicts.

-- 
Regards,
Ivan Khoronzhuk


  parent reply	other threads:[~2015-04-21 16:03 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-20 10:19 [Patch v2 0/3] firmware: dmi_scan: add SBMIOS entry point and DMI tables Ivan Khoronzhuk
2015-04-20 10:19 ` [Patch v2 1/3] firmware: dmi_scan: rename dmi_table to dmi_decode_table Ivan Khoronzhuk
     [not found] ` <1429525187-3376-1-git-send-email-ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
2015-04-20 10:19   ` [Patch v2 2/3] firmware: dmi_scan: add SBMIOS entry and DMI tables Ivan Khoronzhuk
2015-04-20 10:19     ` Ivan Khoronzhuk
     [not found]     ` <1429525187-3376-3-git-send-email-ivan.khoronzhuk-hExfYMNmJl/Cnp4W7fqMDg@public.gmane.org>
2015-04-20 18:32       ` Roy Franz
2015-04-20 18:32         ` Roy Franz
2015-04-21 14:24     ` Jean Delvare
2015-04-21 15:26       ` Ivan.khoronzhuk
2015-04-21 15:36     ` Jean Delvare
2015-04-21 15:36       ` Jean Delvare
     [not found]       ` <20150421173613.6601da15-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org>
2015-04-21 16:03         ` Ivan.khoronzhuk [this message]
2015-04-21 16:03           ` Ivan.khoronzhuk
2015-04-23 11:33           ` Jean Delvare
2015-04-20 10:19   ` [Patch v2 3/3] Documentation: ABI: sysfs-firmware-dmi: add -entries suffix to file name Ivan Khoronzhuk
2015-04-20 10:19     ` Ivan Khoronzhuk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=553674D2.10407@globallogic.com \
    --to=ivan.khoronzhuk-hexfymnmjl/cnp4w7fqmdg@public.gmane.org \
    --cc=ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
    --cc=dmidecode-devel-qX2TKyscuCcdnm+yROfE0A@public.gmane.org \
    --cc=grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
    --cc=jdelvare-l3A5Bk7waGM@public.gmane.org \
    --cc=leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
    --cc=linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \
    --cc=mikew-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org \
    --cc=msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
    --cc=roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.