| Message ID | 20190510223744.10154-3-prsriva02@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Kexec cmdline bufffer measure | expand |
On 5/11/2019 12:37 AM, Prakhar Srivastava wrote: > From: Prakhar Srivastava <prsriva02@gmail.com> > > The buffer(cmdline args) added to the ima log cannot be attested > without having the actual buffer. Thus to make the measured buffer > available to stroe/read a new ima temaplate (buf) is added. Hi Prakhar please fix the typos. More comments below. > The cmdline args used for soft reboot can then be read and attested > later. > > The patch adds a new template field buf to store/read the buffer > used while measuring kexec_cmdline args in the > [PATCH 1/2 v5]: "add a new ima hook and policy to measure the cmdline". > Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com> > --- > security/integrity/ima/ima_main.c | 23 +++++++++++++++++++++++ > security/integrity/ima/ima_template.c | 2 ++ > security/integrity/ima/ima_template_lib.c | 21 +++++++++++++++++++++ > security/integrity/ima/ima_template_lib.h | 4 ++++ > security/integrity/integrity.h | 1 + > 5 files changed, 51 insertions(+) > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 1d186bda25fe..ca12885ca241 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -605,10 +605,32 @@ static int process_buffer_measurement(const void *buf, int size, > int pcr = CONFIG_IMA_MEASURE_PCR_IDX; > int action = 0; > > + struct buffer_xattr { > + enum evm_ima_xattr_type type; > + u16 buf_length; > + unsigned char buf[0]; > + }; > + struct buffer_xattr *buffer_event_data = NULL; > + int alloc_length = 0; > + > action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr); > if (!(action & IMA_AUDIT) && !(action & IMA_MEASURE)) > goto out; > > + alloc_length = sizeof(struct buffer_xattr) + size; > + buffer_event_data = kzalloc(alloc_length, GFP_KERNEL); > + if (!buffer_event_data) { > + ret = -ENOMEM; > + goto out; > + } > + > + buffer_event_data->type = IMA_XATTR_BUFFER; > + buffer_event_data->buf_length = size; > + memcpy(buffer_event_data->buf, buf, size); > + > + event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data; > + event_data.xattr_len = alloc_length; I would prefer that you introduce two new fields in the ima_event_data structure. You can initialize them directly with the parameters of process_buffer_measurement(). ima_write_template_field_data() will make a copy. > + > memset(iint, 0, sizeof(*iint)); > memset(&hash, 0, sizeof(hash)); > > @@ -638,6 +660,7 @@ static int process_buffer_measurement(const void *buf, int size, > } > > out: > + kfree(buffer_event_data); > return ret; > } > > diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c > index b631b8bc7624..a76d1c04162a 100644 > --- a/security/integrity/ima/ima_template.c > +++ b/security/integrity/ima/ima_template.c > @@ -43,6 +43,8 @@ static const struct ima_template_field supported_fields[] = { > .field_show = ima_show_template_string}, > {.field_id = "sig", .field_init = ima_eventsig_init, > .field_show = ima_show_template_sig}, > + {.field_id = "buf", .field_init = ima_eventbuf_init, > + .field_show = ima_show_template_buf}, Please update Documentation/security/IMA-templates.rst Thanks Roberto > }; > #define MAX_TEMPLATE_NAME_LEN 15 > > diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c > index 513b457ae900..95a827f42c18 100644 > --- a/security/integrity/ima/ima_template_lib.c > +++ b/security/integrity/ima/ima_template_lib.c > @@ -162,6 +162,11 @@ void ima_show_template_sig(struct seq_file *m, enum ima_show_type show, > ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data); > } > > +void ima_show_template_buf(struct seq_file *m, enum ima_show_type show, > + struct ima_field_data *field_data) > +{ > + ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data); > +} > /** > * ima_parse_buf() - Parses lengths and data from an input buffer > * @bufstartp: Buffer start address. > @@ -389,3 +394,19 @@ int ima_eventsig_init(struct ima_event_data *event_data, > return ima_write_template_field_data(xattr_value, event_data->xattr_len, > DATA_FMT_HEX, field_data); > } > + > +/* > + * ima_eventbuf_init - include the buffer(kexec-cmldine) as part of the > + * template data. > + */ > +int ima_eventbuf_init(struct ima_event_data *event_data, > + struct ima_field_data *field_data) > +{ > + struct evm_ima_xattr_data *xattr_value = event_data->xattr_value; > + > + if ((!xattr_value) || (xattr_value->type != IMA_XATTR_BUFFER)) > + return 0; > + > + return ima_write_template_field_data(xattr_value, event_data->xattr_len, > + DATA_FMT_HEX, field_data); > +} > diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h > index 6a3d8b831deb..12f1a8578b31 100644 > --- a/security/integrity/ima/ima_template_lib.h > +++ b/security/integrity/ima/ima_template_lib.h > @@ -29,6 +29,8 @@ void ima_show_template_string(struct seq_file *m, enum ima_show_type show, > struct ima_field_data *field_data); > void ima_show_template_sig(struct seq_file *m, enum ima_show_type show, > struct ima_field_data *field_data); > +void ima_show_template_buf(struct seq_file *m, enum ima_show_type show, > + struct ima_field_data *field_data); > int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp, > int maxfields, struct ima_field_data *fields, int *curfields, > unsigned long *len_mask, int enforce_mask, char *bufname); > @@ -42,4 +44,6 @@ int ima_eventname_ng_init(struct ima_event_data *event_data, > struct ima_field_data *field_data); > int ima_eventsig_init(struct ima_event_data *event_data, > struct ima_field_data *field_data); > +int ima_eventbuf_init(struct ima_event_data *event_data, > + struct ima_field_data *field_data); > #endif /* __LINUX_IMA_TEMPLATE_LIB_H */ > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index 7de59f44cba3..14ef904f091d 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -74,6 +74,7 @@ enum evm_ima_xattr_type { > EVM_IMA_XATTR_DIGSIG, > IMA_XATTR_DIGEST_NG, > EVM_XATTR_PORTABLE_DIGSIG, > + IMA_XATTR_BUFFER, > IMA_XATTR_LAST > }; > >
On Mon, May 13, 2019 at 6:48 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > On 5/11/2019 12:37 AM, Prakhar Srivastava wrote: > > From: Prakhar Srivastava <prsriva02@gmail.com> > > > > The buffer(cmdline args) added to the ima log cannot be attested > > without having the actual buffer. Thus to make the measured buffer > > available to store/read a new ima template (buf) is added. > > Hi Prakhar > > please fix the typos. More comments below. > > > > + buffer_event_data->type = IMA_XATTR_BUFFER; > > + buffer_event_data->buf_length = size; > > + memcpy(buffer_event_data->buf, buf, size); > > + > > + event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data; > > + event_data.xattr_len = alloc_length; > > I would prefer that you introduce two new fields in the ima_event_data > structure. You can initialize them directly with the parameters of > process_buffer_measurement(). I will make the edits, this will definitely save the kzalloc in this code path. > > ima_write_template_field_data() will make > a copy. > Since event_data->type is used to distinguish what the template field should contain. Removing the type and subsequent check in the template_init, buf template fmt will result in the whole event_Data structure being added to the log, which is not the expected output. For buffer entries, the buf templet fmt will contains the buffer itself. > > > + .field_show = ima_show_template_buf}, > > Please update Documentation/security/IMA-templates.rst Will update the documentation. Thanks, Prakhar Srivastava > > Thanks > > Roberto
On 5/14/2019 7:07 AM, prakhar srivastava wrote: > On Mon, May 13, 2019 at 6:48 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: >> >> On 5/11/2019 12:37 AM, Prakhar Srivastava wrote: >>> From: Prakhar Srivastava <prsriva02@gmail.com> >>> >>> The buffer(cmdline args) added to the ima log cannot be attested >>> without having the actual buffer. Thus to make the measured buffer >>> available to store/read a new ima template (buf) is added. >> >> Hi Prakhar >> >> please fix the typos. More comments below. >> >> >>> + buffer_event_data->type = IMA_XATTR_BUFFER; >>> + buffer_event_data->buf_length = size; >>> + memcpy(buffer_event_data->buf, buf, size); >>> + >>> + event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data; >>> + event_data.xattr_len = alloc_length; >> >> I would prefer that you introduce two new fields in the ima_event_data >> structure. You can initialize them directly with the parameters of >> process_buffer_measurement(). > I will make the edits, this will definitely save the kzalloc in this code > path. >> >> ima_write_template_field_data() will make >> a copy. >> > Since event_data->type is used to distinguish what the template field > should contain. > Removing the type and subsequent check in the template_init, > buf template fmt will result in the whole event_Data structure > being added to the log, which is not the expected output. > For buffer entries, the buf templet fmt will contains the buffer itself. The purpose of ima_event_data is to pass data to the init method of template fields. Each method takes the data it needs. If you pass event_data->buf and event_data->buf_len to ima_write_template_field_data() this should be fine. Roberto >>> + .field_show = ima_show_template_buf}, >> >> Please update Documentation/security/IMA-templates.rst > Will update the documentation. > > Thanks, > Prakhar Srivastava >> >> Thanks >> >> Roberto
On Tue, May 14, 2019 at 6:22 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > On 5/14/2019 7:07 AM, prakhar srivastava wrote: > > On Mon, May 13, 2019 at 6:48 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > >> > >> On 5/11/2019 12:37 AM, Prakhar Srivastava wrote: > >>> From: Prakhar Srivastava <prsriva02@gmail.com> > >>> > >>> The buffer(cmdline args) added to the ima log cannot be attested > >>> without having the actual buffer. Thus to make the measured buffer > >>> available to store/read a new ima template (buf) is added. > >> > >> Hi Prakhar > >> > >> please fix the typos. More comments below. > >> > >> > >>> + buffer_event_data->type = IMA_XATTR_BUFFER; > >>> + buffer_event_data->buf_length = size; > >>> + memcpy(buffer_event_data->buf, buf, size); > >>> + > >>> + event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data; > >>> + event_data.xattr_len = alloc_length; > >> > >> I would prefer that you introduce two new fields in the ima_event_data > >> structure. You can initialize them directly with the parameters of > >> process_buffer_measurement(). > > I will make the edits, this will definitely save the kzalloc in this code > > path. > >> > >> ima_write_template_field_data() will make > >> a copy. > >> > > Since event_data->type is used to distinguish what the template field > > should contain. > > Removing the type and subsequent check in the template_init, > > buf template fmt will result in the whole event_Data structure > > being added to the log, which is not the expected output. > > For buffer entries, the buf template fmt will contains the buffer itself. > > The purpose of ima_event_data is to pass data to the init method of > template fields. Each method takes the data it needs. > > If you pass event_data->buf and event_data->buf_len to > ima_write_template_field_data() this should be fine. Hi Roberto, I did some testing after making the needed code changes, the output is as expected the buf template field only contains the buf when the ima_event_data.buf is set. However i just want to double check if adding two new fields to the struct ima_event_data is approach you want me to take? Mimi any concerns? what all tests do i need to run to confirm i am not in-inadvertently breaking some thing else? Thanks, Prakhar Srivastava > > Roberto > > > >>> + .field_show = ima_show_template_buf}, > >> > >> Please update Documentation/security/IMA-templates.rst > > Will update the documentation. > > > > Thanks, > > Prakhar Srivastava > >> > >> Thanks > >> > >> Roberto > > -- > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 > Managing Director: Bo PENG, Jian LI, Yanli SHI
On 5/18/2019 1:32 AM, prakhar srivastava wrote: > On Tue, May 14, 2019 at 6:22 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: >> >> On 5/14/2019 7:07 AM, prakhar srivastava wrote: >>> On Mon, May 13, 2019 at 6:48 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: >>>> >>>> On 5/11/2019 12:37 AM, Prakhar Srivastava wrote: >>>>> From: Prakhar Srivastava <prsriva02@gmail.com> >>>>> >>>>> The buffer(cmdline args) added to the ima log cannot be attested >>>>> without having the actual buffer. Thus to make the measured buffer >>>>> available to store/read a new ima template (buf) is added. >>>> >>>> Hi Prakhar >>>> >>>> please fix the typos. More comments below. >>>> >>>> >>>>> + buffer_event_data->type = IMA_XATTR_BUFFER; >>>>> + buffer_event_data->buf_length = size; >>>>> + memcpy(buffer_event_data->buf, buf, size); >>>>> + >>>>> + event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data; >>>>> + event_data.xattr_len = alloc_length; >>>> >>>> I would prefer that you introduce two new fields in the ima_event_data >>>> structure. You can initialize them directly with the parameters of >>>> process_buffer_measurement(). >>> I will make the edits, this will definitely save the kzalloc in this code >>> path. >>>> >>>> ima_write_template_field_data() will make >>>> a copy. >>>> >>> Since event_data->type is used to distinguish what the template field >>> should contain. >>> Removing the type and subsequent check in the template_init, >>> buf template fmt will result in the whole event_Data structure >>> being added to the log, which is not the expected output. >>> For buffer entries, the buf template fmt will contains the buffer itself. > >> >> The purpose of ima_event_data is to pass data to the init method of >> template fields. Each method takes the data it needs. >> >> If you pass event_data->buf and event_data->buf_len to >> ima_write_template_field_data() this should be fine. > > Hi Roberto, > I did some testing after making the needed code changes, > the output is as expected the buf template field only contains > the buf when the ima_event_data.buf is set. > > However i just want to double check if adding two new fields to > the struct ima_event_data is approach you want me to take? > Mimi any concerns? I think it should not be a problem. ima_event_data was introduced to pass more information to a function for a new template field, without changing the definition of existing functions. > what all tests do i need to run to confirm i am not > in-inadvertently breaking some thing else? ima_event_data is not used for marshaling/unmarshaling. Adding two new members to the structure won't change the behavior of existing code. Roberto > Thanks, > Prakhar Srivastava >> >> Roberto >> >> >>>>> + .field_show = ima_show_template_buf}, >>>> >>>> Please update Documentation/security/IMA-templates.rst >>> Will update the documentation. >>> >>> Thanks, >>> Prakhar Srivastava >>>> >>>> Thanks >>>> >>>> Roberto >> >> -- >> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 >> Managing Director: Bo PENG, Jian LI, Yanli SHI
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 1d186bda25fe..ca12885ca241 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -605,10 +605,32 @@ static int process_buffer_measurement(const void *buf, int size, int pcr = CONFIG_IMA_MEASURE_PCR_IDX; int action = 0; + struct buffer_xattr { + enum evm_ima_xattr_type type; + u16 buf_length; + unsigned char buf[0]; + }; + struct buffer_xattr *buffer_event_data = NULL; + int alloc_length = 0; + action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr); if (!(action & IMA_AUDIT) && !(action & IMA_MEASURE)) goto out; + alloc_length = sizeof(struct buffer_xattr) + size; + buffer_event_data = kzalloc(alloc_length, GFP_KERNEL); + if (!buffer_event_data) { + ret = -ENOMEM; + goto out; + } + + buffer_event_data->type = IMA_XATTR_BUFFER; + buffer_event_data->buf_length = size; + memcpy(buffer_event_data->buf, buf, size); + + event_data.xattr_value = (struct evm_ima_xattr_data *)buffer_event_data; + event_data.xattr_len = alloc_length; + memset(iint, 0, sizeof(*iint)); memset(&hash, 0, sizeof(hash)); @@ -638,6 +660,7 @@ static int process_buffer_measurement(const void *buf, int size, } out: + kfree(buffer_event_data); return ret; } diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index b631b8bc7624..a76d1c04162a 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -43,6 +43,8 @@ static const struct ima_template_field supported_fields[] = { .field_show = ima_show_template_string}, {.field_id = "sig", .field_init = ima_eventsig_init, .field_show = ima_show_template_sig}, + {.field_id = "buf", .field_init = ima_eventbuf_init, + .field_show = ima_show_template_buf}, }; #define MAX_TEMPLATE_NAME_LEN 15 diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 513b457ae900..95a827f42c18 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -162,6 +162,11 @@ void ima_show_template_sig(struct seq_file *m, enum ima_show_type show, ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data); } +void ima_show_template_buf(struct seq_file *m, enum ima_show_type show, + struct ima_field_data *field_data) +{ + ima_show_template_field_data(m, show, DATA_FMT_HEX, field_data); +} /** * ima_parse_buf() - Parses lengths and data from an input buffer * @bufstartp: Buffer start address. @@ -389,3 +394,19 @@ int ima_eventsig_init(struct ima_event_data *event_data, return ima_write_template_field_data(xattr_value, event_data->xattr_len, DATA_FMT_HEX, field_data); } + +/* + * ima_eventbuf_init - include the buffer(kexec-cmldine) as part of the + * template data. + */ +int ima_eventbuf_init(struct ima_event_data *event_data, + struct ima_field_data *field_data) +{ + struct evm_ima_xattr_data *xattr_value = event_data->xattr_value; + + if ((!xattr_value) || (xattr_value->type != IMA_XATTR_BUFFER)) + return 0; + + return ima_write_template_field_data(xattr_value, event_data->xattr_len, + DATA_FMT_HEX, field_data); +} diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h index 6a3d8b831deb..12f1a8578b31 100644 --- a/security/integrity/ima/ima_template_lib.h +++ b/security/integrity/ima/ima_template_lib.h @@ -29,6 +29,8 @@ void ima_show_template_string(struct seq_file *m, enum ima_show_type show, struct ima_field_data *field_data); void ima_show_template_sig(struct seq_file *m, enum ima_show_type show, struct ima_field_data *field_data); +void ima_show_template_buf(struct seq_file *m, enum ima_show_type show, + struct ima_field_data *field_data); int ima_parse_buf(void *bufstartp, void *bufendp, void **bufcurp, int maxfields, struct ima_field_data *fields, int *curfields, unsigned long *len_mask, int enforce_mask, char *bufname); @@ -42,4 +44,6 @@ int ima_eventname_ng_init(struct ima_event_data *event_data, struct ima_field_data *field_data); int ima_eventsig_init(struct ima_event_data *event_data, struct ima_field_data *field_data); +int ima_eventbuf_init(struct ima_event_data *event_data, + struct ima_field_data *field_data); #endif /* __LINUX_IMA_TEMPLATE_LIB_H */ diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 7de59f44cba3..14ef904f091d 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -74,6 +74,7 @@ enum evm_ima_xattr_type { EVM_IMA_XATTR_DIGSIG, IMA_XATTR_DIGEST_NG, EVM_XATTR_PORTABLE_DIGSIG, + IMA_XATTR_BUFFER, IMA_XATTR_LAST };