Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 15 Oct 2021 17:25:13 -0700
From:   Stephen Hemminger <stephen@networkplumber.org>
To:     linux-bluetooth@vger.kernel.org
Subject: Fw: [Bug 214729] New: Information leakage from kernel to user space
 in /net/bluetooth/hci_sock.c
Message-ID: <20211015172513.2836ed31@hermes.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


Begin forwarded message:

Date: Fri, 15 Oct 2021 22:14:56 +0000
From: bugzilla-daemon@bugzilla.kernel.org
To: stephen@networkplumber.org
Subject: [Bug 214729] New: Information leakage from kernel to user space in=
 /net/bluetooth/hci_sock.c


https://bugzilla.kernel.org/show_bug.cgi?id=3D214729

            Bug ID: 214729
           Summary: Information leakage from kernel to user space in
                    /net/bluetooth/hci_sock.c
           Product: Networking
           Version: 2.5
    Kernel Version: 5.15-rc5
          Hardware: All
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: high
          Priority: P1
         Component: Other
          Assignee: stephen@networkplumber.org
          Reporter: bao00065@umn.edu
        Regression: No

Hi Maintainers,
I recently reviewed the uninitialized value use bug in Linux kernel:

https://syzkaller.appspot.com/bug?id=3D739ce0bc6e4097668cbf94c862f3b643b364=
d589.=20
and patch:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?=
id=3Db42b3a2744b3e8f427de79896720c72823af91ad

The vulnerable function is:
int __sys_getsockname(int fd, struct sockaddr __user *usockaddr,
                      int __user *usockaddr_len)
{
        struct socket *sock;
        struct sockaddr_storage address; // allocation
        int err, fput_needed;

        sock =3D sockfd_lookup_light(fd, &err, &fput_needed);
        if (!sock)
                goto out;

        err =3D security_socket_getsockname(sock);
        if (err)
                goto out_put;

        err =3D sock->ops->getname(sock, (struct sockaddr *)&address, 0); //
initialization
        if (err < 0)
                goto out_put;
        /* "err" is actually length in this case */
        err =3D move_addr_to_user(&address, err, usockaddr, usockaddr_len);=
 //use

out_put:
        fput_light(sock->file, fput_needed);
out:
        return err;
}

static int move_addr_to_user(struct sockaddr_storage *kaddr, int klen,
                             void __user *uaddr, int __user *ulen)
{
        int err;
        int len;

        BUG_ON(klen > sizeof(struct sockaddr_storage));
        err =3D get_user(len, ulen);
        if (err)
                return err;
        if (len > klen)
                len =3D klen;
        if (len < 0)
                return -EINVAL;
        if (len) {
                if (audit_sockaddr(klen, kaddr))
                        return -ENOMEM;
                if (copy_to_user(uaddr, kaddr, len)) // use=20
                        return -EFAULT;
        }
        /*
         *      "fromlen shall refer to the value before truncation.."
         *                      1003.1g
         */
        return __put_user(klen, ulen);
}


the variable address is allocated in __sys_getsockname, and then is initial=
ized
in sock->ops->getname(sock, (struct sockaddr *)& address, 0); After that,
address is passed to move_addr_to_user() and finally it passed to
copy_to_user(), leading to uninitialized value use.=20

Main reason for this bug: initialization in sock->ops->getname(sock, (struct
sockaddr *)&address, 0) is partially initialized.  It only initializes the
fields in the struct but not the holes between the fields. As a result, sin=
ce
uaddr will be passed to copy_to_user(), and the holes inside the uaddr stru=
ct=20
contain uninitialized data inherited from the kernel stack, it may cause
information leakage from kernel space to user space

Here is the initialization function:

static int isotp_getname(struct socket *sock, struct sockaddr *uaddr, int p=
eer)
{
        struct sockaddr_can *addr =3D (struct sockaddr_can *)uaddr;
        struct sock *sk =3D sock->sk;
        struct isotp_sock *so =3D isotp_sk(sk);

        if (peer)
                return -EOPNOTSUPP;

        +memset(addr, 0, ISOTP_MIN_NAMELEN);//patch
        addr->can_family =3D AF_CAN;
        addr->can_ifindex =3D so->ifindex;
        addr->can_addr.tp.rx_id =3D so->rxid;
        addr->can_addr.tp.tx_id =3D so->txid;

        return ISOTP_MIN_NAMELEN;
}

The patch: memset() initializes the whole struct sockaddr_can, including the
holes within the struct sockaddr_can.

Sockaddr_can is declared here:
struct sockaddr_can {
        __kernel_sa_family_t can_family;
        int         can_ifindex;
        union {
                /* transport protocol class address information (e.g. ISOTP=
) */
                struct { canid_t rx_id, tx_id; } tp;

                /* J1939 address information */
                struct {
                        /* 8 byte name when using dynamic addressing */
                        __u64 name;

                        /* pgn:
                         * 8 bit: PS in PDU2 case, else 0
                         * 8 bit: PF
                         * 1 bit: DP
                         * 1 bit: reserved
                         */
                        __u32 pgn;

                        /* 1 byte address */
                        __u8 addr;
                } j1939;

                /* reserved for future CAN protocols address information */
        } can_addr;
};
There are a few holes inside the struct, but it doesn=E2=80=99t explicitly =
set to 0 in=20
isotp_getname()

At the same time, I realized  sock->ops->getname(sock, (struct sockaddr
*)&address, 0) is an indirect call. Thus it may go to different functions at
the run time. If one of these functions doesn't initialize the holes within=
 the
struct, it may cause an information leak from kernel space to userspace.=20


My tools find similar cloned bugs
The same bug happen in /net/bluetooth/hci_sock.c

s
static int hci_sock_getname(struct socket *sock, struct sockaddr *addr,
                            int peer)
{
        struct sockaddr_hci *haddr =3D (struct sockaddr_hci *)addr;
        struct sock *sk =3D sock->sk;
        struct hci_dev *hdev;
        int err =3D 0;

        BT_DBG("sock %p sk %p", sock, sk);

        if (peer)
                return -EOPNOTSUPP;

        lock_sock(sk);

        hdev =3D hci_hdev_from_sock(sk);
        if (IS_ERR(hdev)) {
                err =3D PTR_ERR(hdev);
                goto done;
        }

        haddr->hci_family =3D AF_BLUETOOTH;
        haddr->hci_dev    =3D hdev->id;
        haddr->hci_channel=3D hci_pi(sk)->channel;
        err =3D sizeof(*haddr);

done:
        release_sock(sk);
        return err;
}
sockaddr_hci is declared here:

struct sockaddr_hci {
        sa_family_t    hci_family;
        unsigned short hci_dev;
        unsigned short hci_channel;
};

We can see there is a hole between family and dev. Thus, we have to explici=
tly
set the hole to zero. Otherwise, the address will be passed to copy_to_user=
 and
cause information leakage.

Suggested patch:
memset(maddr, 0, sizeof(sockaddr_hci));


Thank you for the review. I appreciate your time.=20

Andrew Bao

--=20
You may reply to this email to add a comment.

You are receiving this mail because:
You are the assignee for the bug.