Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20231017182026.2141163-1-danielmentz@google.com>
 <DM6PR04MB6575C45CF299649DF41FD963FCD6A@DM6PR04MB6575.namprd04.prod.outlook.com>
 <52a248fc-465e-4050-8692-5105b6aaa764@acm.org>
In-Reply-To: <52a248fc-465e-4050-8692-5105b6aaa764@acm.org>
From:   Daniel Mentz <danielmentz@google.com>
Date:   Tue, 17 Oct 2023 14:56:33 -0700
Message-ID: <CAE2F3rCcEC=fMd+_gxwksTW=07ZzAt-vA5SO3w8AdAXGy1OVDg@mail.gmail.com>
Subject: Re: [PATCH] scsi: ufs: Leave space for '\0' in utf8 desc string
To:     Bart Van Assche <bvanassche@acm.org>,
        Avri Altman <avri.altman@wdc.com>
Cc:     "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Tomas Winkler <tomas.winkler@intel.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        Mars Cheng <marscheng@google.com>,
        Yen-lin Lai <yenlinlai@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Oct 17, 2023 at 12:33=E2=80=AFPM Bart Van Assche <bvanassche@acm.or=
g> wrote:
>
> On 10/17/23 12:20, Avri Altman wrote:
> >> Fixes: 4b828fe156a6 ("scsi: ufs: revamp string descriptor reading")
> > I think this code goes back to commit b573d484e4ff (scsi: ufs: add supp=
ort to read device and string descriptors)
>
> Hmm ... it seems to me that there was no buffer overflow in commit
> b573d484e4ff but that the buffer overflow was introduced by commit
> 4b828fe156a6?

Thank you for the review Avri.

To me, it appears as if those two commits had different issues:

commit b573d484e4ff ("scsi: ufs: add support to read device and string
descriptors") failed to reliably NULL terminate the output string (in
the case where ascii_len =3D=3D size - QUERY_DESC_HDR_SIZE).

commit 4b828fe156a6 ("scsi: ufs: revamp string descriptor reading")
potentially performs an out-of-bounds array access while NULL
terminating the output string.

I would argue that the proposed fix wouldn't even fix the former and
older commit b573d484e4ff, because that commit might have required
more fixes like using kzalloc instead of kmalloc.
I find that the newer commit 4b828fe156a6 did enough of refactoring
for it to be considered the commit that needs this fix.