utf16s_to_utf8s does not NULL terminate the output string. For us to be
able to add a NULL character when utf16s_to_utf8s returns, we need to
make sure that there is space for such NULL character at the end of the
output buffer. We can achieve this by passing an output buffer size to
utf16s_to_utf8s that is one character less than what we allocated.
Other call sites of utf16s_to_utf8s appear to be using the same
technique where they artificially reduce the buffer size by one to leave
space for a NULL character or line feed character.
Fixes: 4b828fe156a6 ("scsi: ufs: revamp string descriptor reading")
Reviewed-by: Mars Cheng <[email protected]>
Reviewed-by: Bart Van Assche <[email protected]>
Reviewed-by: Yen-lin Lai <[email protected]>
Signed-off-by: Daniel Mentz <[email protected]>
---
drivers/ufs/core/ufshcd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8382e8cfa414..5767642982c1 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3632,7 +3632,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
*/
ret = utf16s_to_utf8s(uc_str->uc,
uc_str->len - QUERY_DESC_HDR_SIZE,
- UTF16_BIG_ENDIAN, str, ascii_len);
+ UTF16_BIG_ENDIAN, str, ascii_len - 1);
/* replace non-printable or non-ASCII characters with spaces */
for (i = 0; i < ret; i++)
--
2.42.0.655.g421f12c284-goog
> utf16s_to_utf8s does not NULL terminate the output string. For us to be able
> to add a NULL character when utf16s_to_utf8s returns, we need to make
> sure that there is space for such NULL character at the end of the output
> buffer. We can achieve this by passing an output buffer size to
> utf16s_to_utf8s that is one character less than what we allocated.
>
> Other call sites of utf16s_to_utf8s appear to be using the same technique
> where they artificially reduce the buffer size by one to leave space for a
> NULL character or line feed character.
>
> Fixes: 4b828fe156a6 ("scsi: ufs: revamp string descriptor reading")
I think this code goes back to commit b573d484e4ff (scsi: ufs: add support to read device and string descriptors)
Reviewed-by: Avri Altman <[email protected]>
> Reviewed-by: Mars Cheng <[email protected]>
> Reviewed-by: Bart Van Assche <[email protected]>
> Reviewed-by: Yen-lin Lai <[email protected]>
> Signed-off-by: Daniel Mentz <[email protected]>
> ---
> drivers/ufs/core/ufshcd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 8382e8cfa414..5767642982c1 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -3632,7 +3632,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba,
> u8 desc_index,
> */
> ret = utf16s_to_utf8s(uc_str->uc,
> uc_str->len - QUERY_DESC_HDR_SIZE,
> - UTF16_BIG_ENDIAN, str, ascii_len);
> + UTF16_BIG_ENDIAN, str, ascii_len -
> + 1);
>
> /* replace non-printable or non-ASCII characters with spaces */
> for (i = 0; i < ret; i++)
> --
> 2.42.0.655.g421f12c284-goog
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 support 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?
Thanks,
Bart.
On Tue, Oct 17, 2023 at 12:33 PM Bart Van Assche <[email protected]> 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 support 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 == 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.
Daniel,
> utf16s_to_utf8s does not NULL terminate the output string. For us to be
> able to add a NULL character when utf16s_to_utf8s returns, we need to
> make sure that there is space for such NULL character at the end of the
> output buffer. We can achieve this by passing an output buffer size to
> utf16s_to_utf8s that is one character less than what we allocated.
Applied to 6.7/scsi-staging, thanks!
--
Martin K. Petersen Oracle Linux Engineering