<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr"> Thank you for the correction John. Despite changing the code to use strdup( ), the kernel still crashes. I have attached the modified file for reference. Is there anything else that might be causing the crash? </div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: John Johansen <john.johansen@canonical.com> Sent: 30 July 2019 04:10:00 To: Abhishek Vijeev <abhishekvijeev@iisc.ac.in>; apparmor@lists.ubuntu.com <apparmor@lists.ubuntu.com> Cc: Rakesh Rajan Beck <rakeshbeck@iisc.ac.in> Subject: Re: [apparmor] Questions about AppArmor's Kernel Code <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">I haven't tested if this is the cause of your failure but it could very well be + // Custom code begin + + if (unpack_nameX(e, AA_STRUCT, "custom_label")) + { + profile->clabel = kzalloc (sizeof(struct custom_label), GFP_KERNEL); + if (!profile->clabel) + goto fail; this block of code + if (!unpack_str(e, &name, NULL)) + goto fail; + profile->clabel->label_name = kzalloc (strlen(name), GFP_KERNEL); + if (!profile->clabel->label_name) + goto fail; + + strcpy (profile->clabel->label_name, name); + you are allocation a string that is not long enough to include the null terminating \0 so when you use strcpy it is writing one byte beyond your allocated data. replace this with + if (!unpack_strdup(e, &profile->clabel->label_name, NULL)) + goto fail; + if (!unpack_u32(e, &(profile->clabel->allow_cnt), NULL)) + goto fail; + + + if (unpack_nameX(e, AA_STRUCT, "data_list")) + { + profile->clabel->allow_list = kzalloc(sizeof(struct data_list), GFP_KERNEL); + if (!profile->clabel->allow_list) + goto fail; + INIT_LIST_HEAD(&(profile->clabel->allow_list->lh)); + + for (i = 0; i < profile->clabel->allow_cnt; i++) + { + if (!unpack_str(e, &name, NULL)) + goto fail; + struct data_list *new_node = kzalloc(sizeof(struct data_list), GFP_KERNEL); + if (!new_node) + goto fail; same thing for this block of code + new_node->data = kzalloc(strlen(name), GFP_KERNEL); + if (!new_node->data) + goto fail; + strcpy(new_node->data, name); its writing 1 past the end of your allocation. You can either - increase allocation size by one to account for terminating \0 - switch to unpack_strdup() like above and add a free on failure + INIT_LIST_HEAD(&(new_node->lh)); + list_add(&(new_node->lh), &(profile->clabel->allow_list->lh)); + } + if (!unpack_nameX(e, AA_STRUCTEND, NULL)) + goto fail; + } + if (!unpack_nameX(e, AA_STRUCTEND, NULL)) + goto fail; + } + + // Custom code end On 7/29/19 12:58 AM, Abhishek Vijeev wrote: > Thank you for the explanation John. > > I have attached the files we have modified. Every piece of code that we inserted is enclosed > within comment lines 'Custom code begin' and 'Custom code end' so that it's easy for you to find. Here > is a brief description of the changes made: > > AppArmor Parser (user-space) - We modified the grammar of AppArmor's parser to include additional > grammar rules. These rules store data in class Profile > > a) profile.h - 2 new structure definitions to store our custom data > - class Profile now contains a member 'clabel' > b) parser_interface.c - Added code to sd_serialize_profile( ) that serializes the additional custom > data we added to class Profile during the parsing phase > AppArmor LSM (kernel) : > > > a) include/policy.h - 2 new structure definitions > - struct aa_profile now contains a member 'clabel' > > b) policy_unpack.c- Added code to unpack_profile( ) that unpacks the serialized object sent from > user-space, and allocates kernel memory for the security structures added to > aa_profile - namely, a label string and a linked list containing allow permissions > c) policy.c - Added code to function aa_free_profile( ) that frees the allocated memory > > ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ > *From:* John Johansen <john.johansen@canonical.com> > *Sent:* 27 July 2019 00:10:14 > *To:* Abhishek Vijeev <abhishekvijeev@iisc.ac.in>; apparmor@lists.ubuntu.com <apparmor@lists.ubuntu.com> > *Cc:* Rakesh Rajan Beck <rakeshbeck@iisc.ac.in> > *Subject:* Re: [apparmor] Questions about AppArmor's Kernel Code > > On 7/26/19 5:56 AM, Abhishek Vijeev wrote: >> Hi, >> >> >> I have a few questions about AppArmor's kernel code and would be grateful if you could kindly answer them. >> >> >> 1) Why does AppArmor maintain two separate security blobs in cred->security as well as task-security for processes? For a simple project that requires associating a security context with every task, would it suffice to use just one of these? >> > the task->security field is used to store task specific information, that is not used for general mediation. Currently the information stored their is for the change_hat and change_onexec apis and some info to track what the confinement was when no-newprivs was applied to the task. > > cred->security is used to store the subjects label (type) for mediation. > > Before the task->security field was reintroduce all the information was stored off the cred in a intermediate structure. Doing so would cause use of the change_hat and change_onexec api to change the cred of the task even when the confinement had not changed. The switch to using the task->security field was pre 4.18 > >> >> 2) There has been a change in the way security blobs are accessed from kernel version 4.18 to 5.2. I see that in v5.2, the security blob's address is obtained by adding the size of the blob to the start address. Why has this change been made? (For reference: <a href="https://github.com/torvalds/linux/blob/master/security/apparmor/include/cred.h#L24">https://github.com/torvalds/linux/blob/master/security/apparmor/include/cred.h#L24</a>) >> > see Casey's answer > >> >> 3) I tried adding a custom field (pointer to a custom structure) to struct aa_profile, at exactly this point - <a href="https://github.com/torvalds/linux/blob/master/security/apparmor/include/policy.h#L144">https://github.com/torvalds/linux/blob/master/security/apparmor/include/policy.h#L144</a>. I have taken care to allocate and free memory for the pointer at the appropriate places (allocation is performed here - <a href="https://github.com/torvalds/linux/blob/master/security/apparmor/policy_unpack.c#L671">https://github.com/torvalds/linux/blob/master/security/apparmor/policy_unpack.c#L671</a> and freeing is performed here - <a href="https://github.com/torvalds/linux/blob/master/security/apparmor/policy.c#L205">https://github.com/torvalds/linux/blob/master/security/apparmor/policy.c#L205</a>). However, while booting the kernel, it crashes at the function 'security_prepare_creds( )', which I presume invokes 'apparmor_cred_prepare( )'. If I was, to assume for a moment that there are no bugs with my memory allocation code, is there any other reason why such a crash might have occurred? I have attached the kernel crash log file with this email for your kind reference. >> > > I know the code points but to be able to comment beyond vague guesses I need to see your changes. I can give you the warning to not add your field after the current last field, > > struct aa_label label; > > as it has a variable length field. While that is always 2 entries when its embedded in the profile the compiler will end up treating it as zero length over lapping your new field with the start of the variable length array. > > I do have a patch to address this using a union but I haven't landed it yet. </div> </div> </body> </html>