Re: [apparmor] Questions about AppArmor's Kernel Code

[Abhishek Vijeev]

Hi Seth,

I'll surely keep that in mind, thank you. Here's the diff for policy_unpack.c 
after incorporating the changes John suggested (strdup( )):

diff --git a/linux-5.2.1/security/apparmor/policy_unpack.c 
b/linux-5.2.1/security/apparmor/policy_unpack.c
index 73836fe94..e63203d29 100644
--- a/linux-5.2.1/security/apparmor/policy_unpack.c
+++ b/linux-5.2.1/security/apparmor/policy_unpack.c
@@ -741,19 +741,14 @@ static struct aa_profile *unpack_profile(struct aa_ext 
*e, char **ns_name)
  profile->clabel = kzalloc (sizeof(struct custom_label), GFP_KERNEL);
  if (!profile->clabel)
  goto fail;
- if (!unpack_str(e, &name, NULL))
+ if (!unpack_strdup(e, &profile->clabel->label_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);

  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)
@@ -762,15 +757,11 @@ static struct aa_profile *unpack_profile(struct aa_ext 
*e, char **ns_name)

  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;
- new_node->data = kzalloc(strlen(name), GFP_KERNEL);
- if (!new_node->data)
- goto fail;
- strcpy(new_node->data, name);
+ if (!unpack_strdup(e, &new_node->data, NULL))
+                    goto fail;
  INIT_LIST_HEAD(&(new_node->lh));
  list_add(&(new_node->lh), &(profile->clabel->allow_list->lh));
  }


I wrongly assumed that the kernel was crashing for the same reasons as before 
incorporating John's changes.

On inspecting the fresh crash logs (attached with this email), I found that the 
crash was now occurring at a different
location - namely the apparmor_socket_sendmsg () function in apparmor/lsm.c. 
Here, I was trying to extract the
destination IP address from struct msghdr *msg, using the following code:

DECLARE_SOCKADDR(struct sockaddr_in *, usin, msg->msg_name);
if (usin)
{
printk (KERN_INFO "msg sent to %pi4, %d\n", usin->sin_addr.s_addr, 
usin->sin_port);
}

Removing this code resulted in a stable kernel, and I have fortunately not had 
a crash since (I'd like to watch for a couple of
more days before being confident that the issue is resolved). However, I'm not 
quite sure what caused the null pointer
de-reference (as suggested by the log file) since I have an explicit check 
against this. Is there a better way to obtain the destination
IP address from struct msghdr *msg?





________________________________
From: Seth Arnold <seth.arn...@canonical.com>
Sent: 31 July 2019 03:55:08
To: Abhishek Vijeev <abhishekvij...@iisc.ac.in>
Cc: apparmor@lists.ubuntu.com <apparmor@lists.ubuntu.com>; Rakesh Rajan Beck 
<rakeshb...@iisc.ac.in>
Subject: Re: [apparmor] Questions about AppArmor's Kernel Code

On Tue, Jul 30, 2019 at 12:42:48PM +0000, Abhishek Vijeev wrote:
> 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?

Hello Abhishek,

Please consider sharing your modification via diffs; passing along entire
files is pretty difficult to read and understand your changes.

Also be sure to include the full kernel messages when trying to debug
crashes. That's the best lever any has to prise apart the problem.

Thanks

dmesg_logs
Description: dmesg_logs

-- 
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/apparmor