linux-kernel - Re: [RFC][Patch] IBM Real-Time "SMI Free" mode driver -v4

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]

Date:	Fri, 24 Sep 2010 09:56:25 -0700
From:	Randy Dunlap <rdunlap@...otime.net>
To:	Vernon Mauery <vernux@...ibm.com>
Cc:	Arnd Bergmann <arnd@...db.de>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Keith Mannthey <kmannth@...ibm.com>
Subject: Re: [RFC][Patch] IBM Real-Time "SMI Free" mode driver -v4

On Fri, 24 Sep 2010 07:14:27 -0700 Vernon Mauery wrote:

> On Fri, Sep 24, 2010 at 6:12 AM, Arnd Bergmann <arnd@...db.de> wrote:
> > On Friday 24 September 2010, Vernon Mauery wrote:
> >> +enum rtl_addr_type {
> >> +     RTL_ADDR_TYPE_IO = 1,
> >> +     RTL_ADDR_TYPE_MMIO,
> >> +} __attribute__((packed));
> >> +
> >> +enum rtl_cmd_type {
> >> +     RTL_CMD_NOP = 0,
> >> +     RTL_CMD_ENTER_PRTM,
> >> +     RTL_CMD_EXIT_PRTM,
> >> +} __attribute__((packed));
> >
> > You didn't reply to Randy's comment about the packed attribute.
> > I think it's rather confusing here.

thanks.

> Sorry.  I missed that comment.  The packed attribute on an enum forces
> it into the smallest int type it can be.  enums are normally the size
> of an int, but this enum in the RTL table must fit in an 8-bit int.

I think you would be better off making 'command' below be a u8 then.
Otherwise someone else might add an enum value that is > 255 and the
struct size will change.  I.e., the current way is error-prone.
Just using u8 for command's type is safer.


> >> +/* The RTL table as presented by the EBDA: */
> >> +struct ibm_rtl_table {
> >> +     char signature[5];
> >> +     u8 version;
> >> +     u8 rt_status;
> >> +     enum rtl_cmd_type command;
> >> +     u8 command_status;
> >> +     enum rtl_addr_type cmd_address_type;
> >> +     u8 cmd_granularity;
> >> +     u8 cmd_offset;
> >> +     u16 reserve1;
> >> +     u8 cmd_port_address[4]; /* platform dependent address */
> >> +     u8 cmd_port_value[4];   /* platform dependent value */
> >> +};
> >
> > I would recommend marking the member in this structure as packed instead,
> > not the enum.
> 
> It does not have the same effect.  Without the packed attribute on the
> enums, they end up to be the wrong size and then we would be reading
> from the wrong location in memory.

Thanks for the explanation.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/