Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 28 Jul 2022 09:40:59 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 2/2] Bluetooth: ISO: fix info leak in
 iso_sock_getsockopt()
Message-ID: <20220728064059.GS2338@kadam>
References: <YuEq2Aey0VOrxPB+@kili>
 <YuErMEjse5lgAMO3@kili>
 <CABBYNZ+bdU4sVjbQfOHtz5P7X+BkZj2DG6f3hNJzdjx+DSVtGw@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CABBYNZ+bdU4sVjbQfOHtz5P7X+BkZj2DG6f3hNJzdjx+DSVtGw@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?9jSzDn9BhmP6RN8RwH7fPpMJR34Ulwf/lI3ZQ2tpWDnH4a5B66tRCnjdeb1S?=
 =?us-ascii?Q?DMdC+aRLeUcUlladaPZ0PiOwOGFcHI1bheCp1N4yb+bG2lks7i5tV57alRW0?=
 =?us-ascii?Q?uerbBQ81zdgkAJ7bFhPkOfvAoFNF8QrFXiwO9jvmg6k0yK28mOCz2dbATxYf?=
 =?us-ascii?Q?37x5UKxcIu4FasSAlOgMfemls5rCpYyTtgoixiIuzbTLXOR19za78c1fKVPE?=
 =?us-ascii?Q?2+zNNUA6rcFDfc3kPZ2W5u62GFmxpoHKE8rchQnmNGEX3sBTh5f3vJg2I/gn?=
 =?us-ascii?Q?qtTUZJQywJA6HhGLD6m5gZ4cNzFFkdGXjg9rRMALkTVp3w7xSAWk3S2pVRDo?=
 =?us-ascii?Q?+iLt9pyOsjggLKqsLoK49kuXJZ9r7S8E+WIxmgmH5pP6BOLJ31CEUe3NBs7j?=
 =?us-ascii?Q?PB7L9bReNhrz8fnjGzL+vwqhH722VspPooBtt/vAmp3mYbsyFWQ/P7cal8TX?=
 =?us-ascii?Q?mhSe71Sng3ZoOTAqlxKlOugbvrwXxQkUGXZp5OeCyhutJhm/kAQgtFcXTk8f?=
 =?us-ascii?Q?6TN9F7zsgzTDV5Dg/i70TIK8R5OKWIH5jm05tzGiWAt2UbuL9B4KTQpJVpoz?=
 =?us-ascii?Q?+pHZuZTW5affWU9a9bBRcum6h7lpDFCYHDrOXeYCU51NYd1Gs1NcUe3NPSrH?=
 =?us-ascii?Q?QGGHpRwfdQfYjdNMHRS7mlwaI/WKALDKAl0y3KqA8b8qM4EaDmF9YlEMGypf?=
 =?us-ascii?Q?LNwYd7dT2AFVVXnKvH51QShAuYmVhASBmw6UxfN9as7HQDzuN2SdSGiD4dy2?=
 =?us-ascii?Q?HXOsKznFd8G+VEkxuvhI+qFpC3rNpzuoCbju3YfLpKWrRkW28g8BGTqWHDqL?=
 =?us-ascii?Q?ehvtUf/r8IFlvDMngWvidSF8lfmktGTb/PzoD8+8C+WmLsgXEh3Zs+L7DX/k?=
 =?us-ascii?Q?ycm76FQmpZzYC+aPpoUO/jtTsrkUc4AWay/h77wnFRD44c2tWciIGyzpZ6qb?=
 =?us-ascii?Q?JYvMZg4x3CLOZ9+bn0q+xVNgYEikEaanoSTju12xFdfHWtCIu8HnqzKWcbq/?=
 =?us-ascii?Q?ECFfiFbDLDbadL21KW72ydU/ghAVq+ymsWV+i7gNzhW38tRzlPr1lKAEHcsY?=
 =?us-ascii?Q?ymgnB3iXByPmsqY4LCNW280BEKs/JP9weuH/ZzSX4sqYId+ouoN+9ukXh5/M?=
 =?us-ascii?Q?PgKQEW1glIJgxNsaMYmQdGXBSJNi4qylw218swhqJs7dWJqhDlY4+mupnQ1U?=
 =?us-ascii?Q?eLiBUPoGVvHRt0dD5/ocErkspAZtj591Fije6zhg9Kxg5NykXQlmSwEEHLBF?=
 =?us-ascii?Q?j98FPZzy9uhUOmuz+XxJF/2vyJwFLS/sHFTS1aADjE3N21Byb15+EdSlvTsU?=
 =?us-ascii?Q?XxeTdMKZJCPQIZB0Mw1WcehABdhPwTPOrVkd9Rv3n9v6aycrhTrcOs/uuBds?=
 =?us-ascii?Q?6KBFicBovNu7oZLbq3F1noQEzCkeqwtPCMkvGkowmOZPpH9WmSXLodx7uzNV?=
 =?us-ascii?Q?64wgYT4rVd42f7BAkojh9CTZACK3AYMMGohOwl1gWcTbLFh0pqU3GWeUSW1w?=
 =?us-ascii?Q?Zbzq000lh1OdFkVzudOi+xMG1XfmqWocKENheq5bnF6ErbeONf70TbioM7gv?=
 =?us-ascii?Q?WD4BoGeYUwxrWWPxZcJHWQ3j2H23e9KVVRRXAVZa?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ac860aa2-b325-4809-45fc-08da706427eb
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jul 2022 06:41:09.7773
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 6qAfkg8qY4EtZ8E0+nkRsPXKG63kI9D4dqXEbKwexbl+h6VRG8YtubtQPjBrq9uuw31YPsfIx96BpvQNdLniaa4y/wkuQJefNfoKItpKhgM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR10MB2558
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-28_01,2022-07-28_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 malwarescore=0
 adultscore=0 mlxscore=0 mlxlogscore=999 suspectscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207280020
X-Proofpoint-ORIG-GUID: pqQVcMQgAlEgJr8YtI8OcKpFJ8quDr3u
X-Proofpoint-GUID: pqQVcMQgAlEgJr8YtI8OcKpFJ8quDr3u
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-bluetooth.vger.kernel.org>
X-Mailing-List: linux-bluetooth@vger.kernel.org

On Wed, Jul 27, 2022 at 12:51:30PM -0700, Luiz Augusto von Dentz wrote:
> Interesting, did you get a report from static analyzer or something?

Yeah.  It's a Smatch check.  Unfortunately, it still complains after my
patch...  Which is frustrating because I thought I had fixed that.

> The variable gets assigned in the code below which has the exact same
> size thus I don't see how it would leave anything uninitialized:
> 
>         if (sk->sk_state == BT_CONNECTED || sk->sk_state == BT_CONNECT2)
>             qos = iso_pi(sk)->conn->hcon->iso_qos;
>         else
>             qos = iso_pi(sk)->qos;

It's the struct holes after ->in and ->out which are the issue.  When
you have an assignment like that, the compiler is allowed to do it as
a series of assignments:

	foo = bar;

becomes:

	foo.a = bar.a;
	foo.b = bar.b;
	foo.c = bar.c;

> 
> Well perhaps it would have been better to use a pointer though so we
> don't have to copy anything:

That works, and it's faster too.  Do you want to send that and give me
a Reported-by tag?  Otherwise I can.

> 
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index ff09c353e64e..0e4ec46ef273 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1233,7 +1233,7 @@ static int iso_sock_getsockopt(struct socket
> *sock, int level, int optname,
>  {
>         struct sock *sk = sock->sk;
>         int len, err = 0;
> -       struct bt_iso_qos qos;
> +       struct bt_iso_qos *qos;
>         u8 base_len;
>         u8 *base;
> 
> @@ -1259,12 +1259,12 @@ static int iso_sock_getsockopt(struct socket
> *sock, int level, int optname,
> 
>         case BT_ISO_QOS:
>                 if (sk->sk_state == BT_CONNECTED || sk->sk_state == BT_CONNECT2)
> -                       qos = iso_pi(sk)->conn->hcon->iso_qos;
> +                       qos = &iso_pi(sk)->conn->hcon->iso_qos;
>                 else
> -                       qos = iso_pi(sk)->qos;
> +                       qos = &iso_pi(sk)->qos;
> 
>                 len = min_t(unsigned int, len, sizeof(qos));
> -               if (copy_to_user(optval, (char *)&qos, len))
> +               if (copy_to_user(optval, (char *)qos, len))

No need to cast btw.

regards,
dan carpenter