Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Nathan Chancellor]

On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
> On 1/26/24 09:51, Greg Kroah-Hartman wrote:
> > On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
> > > On 1/22/24 15:55, Greg Kroah-Hartman wrote:
> > > > This is the start of the stable review cycle for the 5.10.209 release.
> > > > There are 286 patches in this series, all will be posted as a response
> > > > to this one.  If anyone has any issues with these being applied, please
> > > > let me know.
> > > > 
> > > > Responses should be made by Wed, 24 Jan 2024 23:56:49 +0000.
> > > > Anything received after that time might be too late.
> > > > 
> > > [ ... ]
> > > 
> > > > zhenwei pi <pizhenwei@bytedance.com>
> > > >       virtio-crypto: implement RSA algorithm
> > > > 
> > > 
> > > Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
> > > It is quite beyond a bug fix. Also, unless I am really missing something,
> > > the series (or at least this patch) was not applied to v5.15.y, so we now
> > > have functionality in v5.10.y which is not in v5.15.y.
> > 
> > See the commit text, it was a dependency of a later fix and documented
> > as such.
> > 
> > Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
> > gladly accepted :)
> > 
> 
> We reverted the entire series from the merge because it results in a build
> failure for us.
> 
> In file included from /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
> In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
> In file included from /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
> In file included from /home/groeck/src/linux-chromeos/include/linux/string.h:293:
> /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: call to __read_overflow2_field declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>                         __read_overflow2_field(q_size_field, size);

For what it's worth, this is likely self inflicted for chromeos-5.10,
which carries a revert of commit eaafc590053b ("fortify: Explicitly
disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
"fortify: Explicitly disable Clang support""). I don't see the series
that added proper support for clang to fortify in 5.18 that ended with
commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
branch, so this seems somewhat expected.

> I also see that upstream (starting with 6.1) when trying to build it with clang,
> so I guess it is one of those bug-for-bug compatibility things. I really have
> no idea what causes it, or why we don't see the problem when building
> chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
> merging 5.10.209 into it. Making things worse, the problem isn't _always_
> seen. Sometimes I can compile the file in 6.1.y without error, sometimes not.
> I have no idea what triggers the problem.

Have a .config that reproduces it on upstream? I have not personally
seen this warning in my build matrix nor has our continuous-integration
matrix (I don't see it in the warning output at the bottom but that
could have missed something for some reason) in 6.1:

https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/7662499796
https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/7662534888

Reverting this series from 5.10 seems reasonable given your other
comments but if there is still something to sort out upstream, I
definitely want to.

> Of course, on top of all that, the error message is completely useless.

Indeed, outstanding papercut unfortunately:
https://github.com/ClangBuiltLinux/linux/issues/1571

Cheers,
Nathan

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Guenter Roeck]

On 1/26/24 12:34, Nathan Chancellor wrote:
> On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
>> On 1/26/24 09:51, Greg Kroah-Hartman wrote:
>>> On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
>>>> On 1/22/24 15:55, Greg Kroah-Hartman wrote:
>>>>> This is the start of the stable review cycle for the 5.10.209 release.
>>>>> There are 286 patches in this series, all will be posted as a response
>>>>> to this one.  If anyone has any issues with these being applied, please
>>>>> let me know.
>>>>>
>>>>> Responses should be made by Wed, 24 Jan 2024 23:56:49 +0000.
>>>>> Anything received after that time might be too late.
>>>>>
>>>> [ ... ]
>>>>
>>>>> zhenwei pi <pizhenwei@bytedance.com>
>>>>>        virtio-crypto: implement RSA algorithm
>>>>>
>>>>
>>>> Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
>>>> It is quite beyond a bug fix. Also, unless I am really missing something,
>>>> the series (or at least this patch) was not applied to v5.15.y, so we now
>>>> have functionality in v5.10.y which is not in v5.15.y.
>>>
>>> See the commit text, it was a dependency of a later fix and documented
>>> as such.
>>>
>>> Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
>>> gladly accepted :)
>>>
>>
>> We reverted the entire series from the merge because it results in a build
>> failure for us.
>>
>> In file included from /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
>> In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
>> In file included from /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
>> In file included from /home/groeck/src/linux-chromeos/include/linux/string.h:293:
>> /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: call to __read_overflow2_field declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>>                          __read_overflow2_field(q_size_field, size);
> 
> For what it's worth, this is likely self inflicted for chromeos-5.10,
> which carries a revert of commit eaafc590053b ("fortify: Explicitly
> disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
> "fortify: Explicitly disable Clang support""). I don't see the series
> that added proper support for clang to fortify in 5.18 that ended with
> commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
> branch, so this seems somewhat expected.
> 

That explains that ;-). I don't mind if the patches stay in v5.10.y,
we have them reverted anyway.

The revert was a pure process issue, as you may see when looking into
commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
Still, that doesn't explain why the problem exists in 5.18+.

>> I also see that upstream (starting with 6.1) when trying to build it with clang,
>> so I guess it is one of those bug-for-bug compatibility things. I really have
>> no idea what causes it, or why we don't see the problem when building
>> chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
>> merging 5.10.209 into it. Making things worse, the problem isn't _always_
>> seen. Sometimes I can compile the file in 6.1.y without error, sometimes not.
>> I have no idea what triggers the problem.
> 
> Have a .config that reproduces it on upstream? I have not personally
> seen this warning in my build matrix nor has our continuous-integration
> matrix (I don't see it in the warning output at the bottom but that
> could have missed something for some reason) in 6.1:
> 

The following command sequence reproduces the problem for me with all stable
branches starting with 5.18.y (plus mainline).

rm -rf /tmp/crypto-build
mkdir /tmp/crypto-build
make -j CC=clang-15 mrproper >/dev/null 2>&1
make -j O=/tmp/crypto-build CC=clang-15 allmodconfig >/dev/null 2>&1
make -j O=/tmp/crypto-build W=1 CC=clang-15 drivers/crypto/virtio/virtio_crypto_akcipher_algs.o

I tried clang versions 14, 15, and 16. This is with my home system running
Ubuntu 22.04, no ChromeOS or Google specifics/internals involved. For clang-15,
the version is

Ubuntu clang version 15.0.7
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

Guenter

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Greg Kroah-Hartman]

On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:
> On 1/26/24 12:34, Nathan Chancellor wrote:
> > On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
> > > On 1/26/24 09:51, Greg Kroah-Hartman wrote:
> > > > On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
> > > > > On 1/22/24 15:55, Greg Kroah-Hartman wrote:
> > > > > > This is the start of the stable review cycle for the 5.10.209 release.
> > > > > > There are 286 patches in this series, all will be posted as a response
> > > > > > to this one.  If anyone has any issues with these being applied, please
> > > > > > let me know.
> > > > > > 
> > > > > > Responses should be made by Wed, 24 Jan 2024 23:56:49 +0000.
> > > > > > Anything received after that time might be too late.
> > > > > > 
> > > > > [ ... ]
> > > > > 
> > > > > > zhenwei pi <pizhenwei@bytedance.com>
> > > > > >        virtio-crypto: implement RSA algorithm
> > > > > > 
> > > > > 
> > > > > Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
> > > > > It is quite beyond a bug fix. Also, unless I am really missing something,
> > > > > the series (or at least this patch) was not applied to v5.15.y, so we now
> > > > > have functionality in v5.10.y which is not in v5.15.y.
> > > > 
> > > > See the commit text, it was a dependency of a later fix and documented
> > > > as such.
> > > > 
> > > > Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
> > > > gladly accepted :)
> > > > 
> > > 
> > > We reverted the entire series from the merge because it results in a build
> > > failure for us.
> > > 
> > > In file included from /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
> > > In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
> > > In file included from /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
> > > In file included from /home/groeck/src/linux-chromeos/include/linux/string.h:293:
> > > /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: call to __read_overflow2_field declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
> > >                          __read_overflow2_field(q_size_field, size);
> > 
> > For what it's worth, this is likely self inflicted for chromeos-5.10,
> > which carries a revert of commit eaafc590053b ("fortify: Explicitly
> > disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
> > "fortify: Explicitly disable Clang support""). I don't see the series
> > that added proper support for clang to fortify in 5.18 that ended with
> > commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
> > branch, so this seems somewhat expected.
> > 
> 
> That explains that ;-). I don't mind if the patches stay in v5.10.y,
> we have them reverted anyway.

Ok, I'll leave them as-is for now, thanks.

greg k-h

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Nathan Chancellor]

(slimming up the CC list, I don't think this is too relevant to the
wider stable community)

On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:
> On 1/26/24 12:34, Nathan Chancellor wrote:
> > On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
> > > On 1/26/24 09:51, Greg Kroah-Hartman wrote:
> > > > On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
> > > > > On 1/22/24 15:55, Greg Kroah-Hartman wrote:
> > > > > > This is the start of the stable review cycle for the 5.10.209 release.
> > > > > > There are 286 patches in this series, all will be posted as a response
> > > > > > to this one.  If anyone has any issues with these being applied, please
> > > > > > let me know.
> > > > > > 
> > > > > > Responses should be made by Wed, 24 Jan 2024 23:56:49 +0000.
> > > > > > Anything received after that time might be too late.
> > > > > > 
> > > > > [ ... ]
> > > > > 
> > > > > > zhenwei pi <pizhenwei@bytedance.com>
> > > > > >        virtio-crypto: implement RSA algorithm
> > > > > > 
> > > > > 
> > > > > Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
> > > > > It is quite beyond a bug fix. Also, unless I am really missing something,
> > > > > the series (or at least this patch) was not applied to v5.15.y, so we now
> > > > > have functionality in v5.10.y which is not in v5.15.y.
> > > > 
> > > > See the commit text, it was a dependency of a later fix and documented
> > > > as such.
> > > > 
> > > > Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
> > > > gladly accepted :)
> > > > 
> > > 
> > > We reverted the entire series from the merge because it results in a build
> > > failure for us.
> > > 
> > > In file included from /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
> > > In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
> > > In file included from /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
> > > In file included from /home/groeck/src/linux-chromeos/include/linux/string.h:293:
> > > /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: call to __read_overflow2_field declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
> > >                          __read_overflow2_field(q_size_field, size);
> > 
> > For what it's worth, this is likely self inflicted for chromeos-5.10,
> > which carries a revert of commit eaafc590053b ("fortify: Explicitly
> > disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
> > "fortify: Explicitly disable Clang support""). I don't see the series
> > that added proper support for clang to fortify in 5.18 that ended with
> > commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
> > branch, so this seems somewhat expected.
> > 
> 
> That explains that ;-). I don't mind if the patches stay in v5.10.y,
> we have them reverted anyway.
> 
> The revert was a pure process issue, as you may see when looking into
> commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
> Still, that doesn't explain why the problem exists in 5.18+.
> 
> > > I also see that upstream (starting with 6.1) when trying to build it with clang,
> > > so I guess it is one of those bug-for-bug compatibility things. I really have
> > > no idea what causes it, or why we don't see the problem when building
> > > chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
> > > merging 5.10.209 into it. Making things worse, the problem isn't _always_
> > > seen. Sometimes I can compile the file in 6.1.y without error, sometimes not.
> > > I have no idea what triggers the problem.
> > 
> > Have a .config that reproduces it on upstream? I have not personally
> > seen this warning in my build matrix nor has our continuous-integration
> > matrix (I don't see it in the warning output at the bottom but that
> > could have missed something for some reason) in 6.1:
> > 
> 
> The following command sequence reproduces the problem for me with all stable
> branches starting with 5.18.y (plus mainline).
> 
> rm -rf /tmp/crypto-build
> mkdir /tmp/crypto-build
> make -j CC=clang-15 mrproper >/dev/null 2>&1
> make -j O=/tmp/crypto-build CC=clang-15 allmodconfig >/dev/null 2>&1
> make -j O=/tmp/crypto-build W=1 CC=clang-15 drivers/crypto/virtio/virtio_crypto_akcipher_algs.o
> 
> I tried clang versions 14, 15, and 16. This is with my home system running
> Ubuntu 22.04, no ChromeOS or Google specifics/internals involved. For clang-15,
> the version is
> 
> Ubuntu clang version 15.0.7
> Target: x86_64-pc-linux-gnu
> Thread model: posix
> InstalledDir: /usr/bin

Okay interesting, this warning is hidden behind W=1, which our CI does
not test with. Looks like it has been that way since the introduction of
these checks in f68f2ff91512 ("fortify: Detect struct member overflows
in memcpy() at compile-time").

I think this is a legitimate warning though. It is complaining about the
second memcpy() in virtio_crypto_alg_akcipher_init_session():

  memcpy(&ctrl->u, para, sizeof(ctrl->u));

where ctrl is:

  struct virtio_crypto_op_ctrl_req {
          struct virtio_crypto_ctrl_header header;         /*     0    16 */
          union {
                  struct virtio_crypto_sym_create_session_req sym_create_session; /*    16    56 */
                  struct virtio_crypto_hash_create_session_req hash_create_session; /*    16    56 */
                  struct virtio_crypto_mac_create_session_req mac_create_session; /*    16    56 */
                  struct virtio_crypto_aead_create_session_req aead_create_session; /*    16    56 */
                  struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /*    16    56 */
                  struct virtio_crypto_destroy_session_req destroy_session; /*    16    56 */
                  __u8               padding[56];          /*    16    56 */
          } u;                                             /*    16    56 */
          union {
                  struct virtio_crypto_sym_create_session_req sym_create_session; /*     0    56 */
                  struct virtio_crypto_hash_create_session_req hash_create_session; /*     0    56 */
                  struct virtio_crypto_mac_create_session_req mac_create_session; /*     0    56 */
                  struct virtio_crypto_aead_create_session_req aead_create_session; /*     0    56 */
                  struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /*     0    56 */
                  struct virtio_crypto_destroy_session_req destroy_session; /*     0    56 */
                  __u8                       padding[56];          /*     0    56 */
          };


          /* size: 72, cachelines: 2, members: 2 */
          /* last cacheline: 8 bytes */
  };

(so size and p_size_field should be 56) and the type of the para
parameter in virtio_crypto_alg_akcipher_init_session() is 'void *' but
the para passed by reference to
virtio_crypto_alg_akcipher_init_session() in virtio_crypto_rsa_set_key()
has a type of 'struct virtio_crypto_akcipher_session_para':

  struct virtio_crypto_akcipher_session_para {
          __le32                     algo;                 /*     0     4 */
          __le32                     keytype;              /*     4     4 */
          __le32                     keylen;               /*     8     4 */
          union {
                  struct virtio_crypto_rsa_session_para rsa; /*    12     8 */
                  struct virtio_crypto_ecdsa_session_para ecdsa; /*    12     8 */
          } u;                                             /*    12     8 */
          union {
                  struct virtio_crypto_rsa_session_para rsa;       /*     0     8 */
                  struct virtio_crypto_ecdsa_session_para ecdsa;   /*     0     8 */
          };


          /* size: 20, cachelines: 1, members: 4 */
          /* last cacheline: 20 bytes */
  };

(so q_size_field would be 20 if clang were able to do inlining to see
through the 'void *'...?), which would result in the

  __compiletime_lessthan(q_size_field, size)

check succeeding and triggering the warning because 20 < 56, so it does
seem like there is an overread of the source buffer here? Adding the
maintainers of the driver and subsystem in question.

Cheers,
Nathan

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Guenter Roeck]

On 1/26/24 14:35, Nathan Chancellor wrote:
> (slimming up the CC list, I don't think this is too relevant to the
> wider stable community)
> 
> On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:
>> On 1/26/24 12:34, Nathan Chancellor wrote:
>>> On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
>>>> On 1/26/24 09:51, Greg Kroah-Hartman wrote:
>>>>> On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
>>>>>> On 1/22/24 15:55, Greg Kroah-Hartman wrote:
>>>>>>> This is the start of the stable review cycle for the 5.10.209 release.
>>>>>>> There are 286 patches in this series, all will be posted as a response
>>>>>>> to this one.  If anyone has any issues with these being applied, please
>>>>>>> let me know.
>>>>>>>
>>>>>>> Responses should be made by Wed, 24 Jan 2024 23:56:49 +0000.
>>>>>>> Anything received after that time might be too late.
>>>>>>>
>>>>>> [ ... ]
>>>>>>
>>>>>>> zhenwei pi <pizhenwei@bytedance.com>
>>>>>>>         virtio-crypto: implement RSA algorithm
>>>>>>>
>>>>>>
>>>>>> Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
>>>>>> It is quite beyond a bug fix. Also, unless I am really missing something,
>>>>>> the series (or at least this patch) was not applied to v5.15.y, so we now
>>>>>> have functionality in v5.10.y which is not in v5.15.y.
>>>>>
>>>>> See the commit text, it was a dependency of a later fix and documented
>>>>> as such.
>>>>>
>>>>> Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
>>>>> gladly accepted :)
>>>>>
>>>>
>>>> We reverted the entire series from the merge because it results in a build
>>>> failure for us.
>>>>
>>>> In file included from /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
>>>> In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
>>>> In file included from /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
>>>> In file included from /home/groeck/src/linux-chromeos/include/linux/string.h:293:
>>>> /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: call to __read_overflow2_field declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>>>>                           __read_overflow2_field(q_size_field, size);
>>>
>>> For what it's worth, this is likely self inflicted for chromeos-5.10,
>>> which carries a revert of commit eaafc590053b ("fortify: Explicitly
>>> disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
>>> "fortify: Explicitly disable Clang support""). I don't see the series
>>> that added proper support for clang to fortify in 5.18 that ended with
>>> commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
>>> branch, so this seems somewhat expected.
>>>
>>
>> That explains that ;-). I don't mind if the patches stay in v5.10.y,
>> we have them reverted anyway.
>>
>> The revert was a pure process issue, as you may see when looking into
>> commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
>> Still, that doesn't explain why the problem exists in 5.18+.
>>
>>>> I also see that upstream (starting with 6.1) when trying to build it with clang,
>>>> so I guess it is one of those bug-for-bug compatibility things. I really have
>>>> no idea what causes it, or why we don't see the problem when building
>>>> chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
>>>> merging 5.10.209 into it. Making things worse, the problem isn't _always_
>>>> seen. Sometimes I can compile the file in 6.1.y without error, sometimes not.
>>>> I have no idea what triggers the problem.
>>>
>>> Have a .config that reproduces it on upstream? I have not personally
>>> seen this warning in my build matrix nor has our continuous-integration
>>> matrix (I don't see it in the warning output at the bottom but that
>>> could have missed something for some reason) in 6.1:
>>>
>>
>> The following command sequence reproduces the problem for me with all stable
>> branches starting with 5.18.y (plus mainline).
>>
>> rm -rf /tmp/crypto-build
>> mkdir /tmp/crypto-build
>> make -j CC=clang-15 mrproper >/dev/null 2>&1
>> make -j O=/tmp/crypto-build CC=clang-15 allmodconfig >/dev/null 2>&1
>> make -j O=/tmp/crypto-build W=1 CC=clang-15 drivers/crypto/virtio/virtio_crypto_akcipher_algs.o
>>
>> I tried clang versions 14, 15, and 16. This is with my home system running
>> Ubuntu 22.04, no ChromeOS or Google specifics/internals involved. For clang-15,
>> the version is
>>
>> Ubuntu clang version 15.0.7
>> Target: x86_64-pc-linux-gnu
>> Thread model: posix
>> InstalledDir: /usr/bin
> 
> Okay interesting, this warning is hidden behind W=1, which our CI does
> not test with. Looks like it has been that way since the introduction of
> these checks in f68f2ff91512 ("fortify: Detect struct member overflows
> in memcpy() at compile-time").
> 

Interestingly the warning is seen in chromeos-5.10, without this patch,
and without W=1. I guess that must have to do with the revert which is
finally biting us.

> I think this is a legitimate warning though. It is complaining about the
> second memcpy() in virtio_crypto_alg_akcipher_init_session():
> 
>    memcpy(&ctrl->u, para, sizeof(ctrl->u));
> 
> where ctrl is:
> 
>    struct virtio_crypto_op_ctrl_req {
>            struct virtio_crypto_ctrl_header header;         /*     0    16 */
>            union {
>                    struct virtio_crypto_sym_create_session_req sym_create_session; /*    16    56 */
>                    struct virtio_crypto_hash_create_session_req hash_create_session; /*    16    56 */
>                    struct virtio_crypto_mac_create_session_req mac_create_session; /*    16    56 */
>                    struct virtio_crypto_aead_create_session_req aead_create_session; /*    16    56 */
>                    struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /*    16    56 */
>                    struct virtio_crypto_destroy_session_req destroy_session; /*    16    56 */
>                    __u8               padding[56];          /*    16    56 */
>            } u;                                             /*    16    56 */
>            union {
>                    struct virtio_crypto_sym_create_session_req sym_create_session; /*     0    56 */
>                    struct virtio_crypto_hash_create_session_req hash_create_session; /*     0    56 */
>                    struct virtio_crypto_mac_create_session_req mac_create_session; /*     0    56 */
>                    struct virtio_crypto_aead_create_session_req aead_create_session; /*     0    56 */
>                    struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /*     0    56 */
>                    struct virtio_crypto_destroy_session_req destroy_session; /*     0    56 */
>                    __u8                       padding[56];          /*     0    56 */
>            };
> 
> 
>            /* size: 72, cachelines: 2, members: 2 */
>            /* last cacheline: 8 bytes */
>    };
> 
> (so size and p_size_field should be 56) and the type of the para
> parameter in virtio_crypto_alg_akcipher_init_session() is 'void *' but
> the para passed by reference to
> virtio_crypto_alg_akcipher_init_session() in virtio_crypto_rsa_set_key()
> has a type of 'struct virtio_crypto_akcipher_session_para':
> 
>    struct virtio_crypto_akcipher_session_para {
>            __le32                     algo;                 /*     0     4 */
>            __le32                     keytype;              /*     4     4 */
>            __le32                     keylen;               /*     8     4 */
>            union {
>                    struct virtio_crypto_rsa_session_para rsa; /*    12     8 */
>                    struct virtio_crypto_ecdsa_session_para ecdsa; /*    12     8 */
>            } u;                                             /*    12     8 */
>            union {
>                    struct virtio_crypto_rsa_session_para rsa;       /*     0     8 */
>                    struct virtio_crypto_ecdsa_session_para ecdsa;   /*     0     8 */
>            };
> 
> 
>            /* size: 20, cachelines: 1, members: 4 */
>            /* last cacheline: 20 bytes */
>    };
> 
> (so q_size_field would be 20 if clang were able to do inlining to see
> through the 'void *'...?), which would result in the
> 
>    __compiletime_lessthan(q_size_field, size)
> 
> check succeeding and triggering the warning because 20 < 56, so it does
> seem like there is an overread of the source buffer here? Adding the

Looks like it; I think either the passed 'para' should be of type
virtio_crypto_akcipher_create_session_req() or it should only copy
sizeof(struct virtio_crypto_akcipher_session_para) bytes.

Anyway, how did you find that ? Is there a magic trick to find the
actual code causing the warning ? I am asking because we had seen
similar warnings before, and it would help to know how to find the
problematic code.

Thanks,
Guenter

> maintainers of the driver and subsystem in question.
> 
> Cheers,
> Nathan

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Nathan Chancellor]

On Fri, Jan 26, 2024 at 03:55:02PM -0800, Guenter Roeck wrote:
> Anyway, how did you find that ? Is there a magic trick to find the
> actual code causing the warning ? I am asking because we had seen
> similar warnings before, and it would help to know how to find the
> problematic code.

The easiest way I have found is figuring out what primitive is causing
the warning (memset, memcpy) then just commenting out the uses in the
particular file until the warning goes away. Sometimes it is quick like
in this case since there were only two instances of memcpy() in that
file but other cases it can definitely take time. There could be
potential issues with that approach if the problematic use is in a
header, at which point you could generate a preprocessed ('.i') file and
see where fortify_memcpy_chk() or fortify_memset_chk() come from in that
file.

Cheers,
Nathan

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Michael S. Tsirkin]

On Fri, Jan 26, 2024 at 03:55:02PM -0800, Guenter Roeck wrote:
> On 1/26/24 14:35, Nathan Chancellor wrote:
> > (slimming up the CC list, I don't think this is too relevant to the
> > wider stable community)
> > 
> > On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:
> > > On 1/26/24 12:34, Nathan Chancellor wrote:
> > > > On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
> > > > > On 1/26/24 09:51, Greg Kroah-Hartman wrote:
> > > > > > On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
> > > > > > > On 1/22/24 15:55, Greg Kroah-Hartman wrote:
> > > > > > > > This is the start of the stable review cycle for the 5.10.209 release.
> > > > > > > > There are 286 patches in this series, all will be posted as a response
> > > > > > > > to this one.  If anyone has any issues with these being applied, please
> > > > > > > > let me know.
> > > > > > > > 
> > > > > > > > Responses should be made by Wed, 24 Jan 2024 23:56:49 +0000.
> > > > > > > > Anything received after that time might be too late.
> > > > > > > > 
> > > > > > > [ ... ]
> > > > > > > 
> > > > > > > > zhenwei pi <pizhenwei@bytedance.com>
> > > > > > > >         virtio-crypto: implement RSA algorithm
> > > > > > > > 
> > > > > > > 
> > > > > > > Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
> > > > > > > It is quite beyond a bug fix. Also, unless I am really missing something,
> > > > > > > the series (or at least this patch) was not applied to v5.15.y, so we now
> > > > > > > have functionality in v5.10.y which is not in v5.15.y.
> > > > > > 
> > > > > > See the commit text, it was a dependency of a later fix and documented
> > > > > > as such.
> > > > > > 
> > > > > > Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
> > > > > > gladly accepted :)
> > > > > > 
> > > > > 
> > > > > We reverted the entire series from the merge because it results in a build
> > > > > failure for us.
> > > > > 
> > > > > In file included from /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
> > > > > In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
> > > > > In file included from /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
> > > > > In file included from /home/groeck/src/linux-chromeos/include/linux/string.h:293:
> > > > > /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: call to __read_overflow2_field declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
> > > > >                           __read_overflow2_field(q_size_field, size);
> > > > 
> > > > For what it's worth, this is likely self inflicted for chromeos-5.10,
> > > > which carries a revert of commit eaafc590053b ("fortify: Explicitly
> > > > disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
> > > > "fortify: Explicitly disable Clang support""). I don't see the series
> > > > that added proper support for clang to fortify in 5.18 that ended with
> > > > commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
> > > > branch, so this seems somewhat expected.
> > > > 
> > > 
> > > That explains that ;-). I don't mind if the patches stay in v5.10.y,
> > > we have them reverted anyway.
> > > 
> > > The revert was a pure process issue, as you may see when looking into
> > > commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
> > > Still, that doesn't explain why the problem exists in 5.18+.
> > > 
> > > > > I also see that upstream (starting with 6.1) when trying to build it with clang,
> > > > > so I guess it is one of those bug-for-bug compatibility things. I really have
> > > > > no idea what causes it, or why we don't see the problem when building
> > > > > chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
> > > > > merging 5.10.209 into it. Making things worse, the problem isn't _always_
> > > > > seen. Sometimes I can compile the file in 6.1.y without error, sometimes not.
> > > > > I have no idea what triggers the problem.
> > > > 
> > > > Have a .config that reproduces it on upstream? I have not personally
> > > > seen this warning in my build matrix nor has our continuous-integration
> > > > matrix (I don't see it in the warning output at the bottom but that
> > > > could have missed something for some reason) in 6.1:
> > > > 
> > > 
> > > The following command sequence reproduces the problem for me with all stable
> > > branches starting with 5.18.y (plus mainline).
> > > 
> > > rm -rf /tmp/crypto-build
> > > mkdir /tmp/crypto-build
> > > make -j CC=clang-15 mrproper >/dev/null 2>&1
> > > make -j O=/tmp/crypto-build CC=clang-15 allmodconfig >/dev/null 2>&1
> > > make -j O=/tmp/crypto-build W=1 CC=clang-15 drivers/crypto/virtio/virtio_crypto_akcipher_algs.o
> > > 
> > > I tried clang versions 14, 15, and 16. This is with my home system running
> > > Ubuntu 22.04, no ChromeOS or Google specifics/internals involved. For clang-15,
> > > the version is
> > > 
> > > Ubuntu clang version 15.0.7
> > > Target: x86_64-pc-linux-gnu
> > > Thread model: posix
> > > InstalledDir: /usr/bin
> > 
> > Okay interesting, this warning is hidden behind W=1, which our CI does
> > not test with. Looks like it has been that way since the introduction of
> > these checks in f68f2ff91512 ("fortify: Detect struct member overflows
> > in memcpy() at compile-time").
> > 
> 
> Interestingly the warning is seen in chromeos-5.10, without this patch,
> and without W=1. I guess that must have to do with the revert which is
> finally biting us.
> 
> > I think this is a legitimate warning though. It is complaining about the
> > second memcpy() in virtio_crypto_alg_akcipher_init_session():
> > 
> >    memcpy(&ctrl->u, para, sizeof(ctrl->u));
> > 
> > where ctrl is:
> > 
> >    struct virtio_crypto_op_ctrl_req {
> >            struct virtio_crypto_ctrl_header header;         /*     0    16 */
> >            union {
> >                    struct virtio_crypto_sym_create_session_req sym_create_session; /*    16    56 */
> >                    struct virtio_crypto_hash_create_session_req hash_create_session; /*    16    56 */
> >                    struct virtio_crypto_mac_create_session_req mac_create_session; /*    16    56 */
> >                    struct virtio_crypto_aead_create_session_req aead_create_session; /*    16    56 */
> >                    struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /*    16    56 */
> >                    struct virtio_crypto_destroy_session_req destroy_session; /*    16    56 */
> >                    __u8               padding[56];          /*    16    56 */
> >            } u;                                             /*    16    56 */
> >            union {
> >                    struct virtio_crypto_sym_create_session_req sym_create_session; /*     0    56 */
> >                    struct virtio_crypto_hash_create_session_req hash_create_session; /*     0    56 */
> >                    struct virtio_crypto_mac_create_session_req mac_create_session; /*     0    56 */
> >                    struct virtio_crypto_aead_create_session_req aead_create_session; /*     0    56 */
> >                    struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /*     0    56 */
> >                    struct virtio_crypto_destroy_session_req destroy_session; /*     0    56 */
> >                    __u8                       padding[56];          /*     0    56 */
> >            };
> > 
> > 
> >            /* size: 72, cachelines: 2, members: 2 */
> >            /* last cacheline: 8 bytes */
> >    };
> > 
> > (so size and p_size_field should be 56) and the type of the para
> > parameter in virtio_crypto_alg_akcipher_init_session() is 'void *' but
> > the para passed by reference to
> > virtio_crypto_alg_akcipher_init_session() in virtio_crypto_rsa_set_key()
> > has a type of 'struct virtio_crypto_akcipher_session_para':
> > 
> >    struct virtio_crypto_akcipher_session_para {
> >            __le32                     algo;                 /*     0     4 */
> >            __le32                     keytype;              /*     4     4 */
> >            __le32                     keylen;               /*     8     4 */
> >            union {
> >                    struct virtio_crypto_rsa_session_para rsa; /*    12     8 */
> >                    struct virtio_crypto_ecdsa_session_para ecdsa; /*    12     8 */
> >            } u;                                             /*    12     8 */
> >            union {
> >                    struct virtio_crypto_rsa_session_para rsa;       /*     0     8 */
> >                    struct virtio_crypto_ecdsa_session_para ecdsa;   /*     0     8 */
> >            };
> > 
> > 
> >            /* size: 20, cachelines: 1, members: 4 */
> >            /* last cacheline: 20 bytes */
> >    };
> > 
> > (so q_size_field would be 20 if clang were able to do inlining to see
> > through the 'void *'...?), which would result in the
> > 
> >    __compiletime_lessthan(q_size_field, size)
> > 
> > check succeeding and triggering the warning because 20 < 56, so it does
> > seem like there is an overread of the source buffer here? Adding the
> 
> Looks like it; I think either the passed 'para' should be of type
> virtio_crypto_akcipher_create_session_req() or it should only copy
> sizeof(struct virtio_crypto_akcipher_session_para) bytes.
> 
> Anyway, how did you find that ? Is there a magic trick to find the
> actual code causing the warning ? I am asking because we had seen
> similar warnings before, and it would help to know how to find the
> problematic code.
> 
> Thanks,
> Guenter
> 
> > maintainers of the driver and subsystem in question.
> > 
> > Cheers,
> > Nathan


CC patch contributor before I revert the whole thing.


-- 
MST

Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[Michael S. Tsirkin]

On Fri, Jan 26, 2024 at 03:55:02PM -0800, Guenter Roeck wrote:
> On 1/26/24 14:35, Nathan Chancellor wrote:
> > (slimming up the CC list, I don't think this is too relevant to the
> > wider stable community)
> > 
> > On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:
> > > On 1/26/24 12:34, Nathan Chancellor wrote:
> > > > On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
> > > > > On 1/26/24 09:51, Greg Kroah-Hartman wrote:
> > > > > > On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
> > > > > > > On 1/22/24 15:55, Greg Kroah-Hartman wrote:
> > > > > > > > This is the start of the stable review cycle for the 5.10.209 release.
> > > > > > > > There are 286 patches in this series, all will be posted as a response
> > > > > > > > to this one.  If anyone has any issues with these being applied, please
> > > > > > > > let me know.
> > > > > > > > 
> > > > > > > > Responses should be made by Wed, 24 Jan 2024 23:56:49 +0000.
> > > > > > > > Anything received after that time might be too late.
> > > > > > > > 
> > > > > > > [ ... ]
> > > > > > > 
> > > > > > > > zhenwei pi <pizhenwei@bytedance.com>
> > > > > > > >         virtio-crypto: implement RSA algorithm
> > > > > > > > 
> > > > > > > 
> > > > > > > Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
> > > > > > > It is quite beyond a bug fix. Also, unless I am really missing something,
> > > > > > > the series (or at least this patch) was not applied to v5.15.y, so we now
> > > > > > > have functionality in v5.10.y which is not in v5.15.y.
> > > > > > 
> > > > > > See the commit text, it was a dependency of a later fix and documented
> > > > > > as such.
> > > > > > 
> > > > > > Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
> > > > > > gladly accepted :)
> > > > > > 
> > > > > 
> > > > > We reverted the entire series from the merge because it results in a build
> > > > > failure for us.
> > > > > 
> > > > > In file included from /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
> > > > > In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
> > > > > In file included from /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
> > > > > In file included from /home/groeck/src/linux-chromeos/include/linux/string.h:293:
> > > > > /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: call to __read_overflow2_field declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
> > > > >                           __read_overflow2_field(q_size_field, size);
> > > > 
> > > > For what it's worth, this is likely self inflicted for chromeos-5.10,
> > > > which carries a revert of commit eaafc590053b ("fortify: Explicitly
> > > > disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
> > > > "fortify: Explicitly disable Clang support""). I don't see the series
> > > > that added proper support for clang to fortify in 5.18 that ended with
> > > > commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
> > > > branch, so this seems somewhat expected.
> > > > 
> > > 
> > > That explains that ;-). I don't mind if the patches stay in v5.10.y,
> > > we have them reverted anyway.
> > > 
> > > The revert was a pure process issue, as you may see when looking into
> > > commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
> > > Still, that doesn't explain why the problem exists in 5.18+.
> > > 
> > > > > I also see that upstream (starting with 6.1) when trying to build it with clang,
> > > > > so I guess it is one of those bug-for-bug compatibility things. I really have
> > > > > no idea what causes it, or why we don't see the problem when building
> > > > > chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
> > > > > merging 5.10.209 into it. Making things worse, the problem isn't _always_
> > > > > seen. Sometimes I can compile the file in 6.1.y without error, sometimes not.
> > > > > I have no idea what triggers the problem.
> > > > 
> > > > Have a .config that reproduces it on upstream? I have not personally
> > > > seen this warning in my build matrix nor has our continuous-integration
> > > > matrix (I don't see it in the warning output at the bottom but that
> > > > could have missed something for some reason) in 6.1:
> > > > 
> > > 
> > > The following command sequence reproduces the problem for me with all stable
> > > branches starting with 5.18.y (plus mainline).
> > > 
> > > rm -rf /tmp/crypto-build
> > > mkdir /tmp/crypto-build
> > > make -j CC=clang-15 mrproper >/dev/null 2>&1
> > > make -j O=/tmp/crypto-build CC=clang-15 allmodconfig >/dev/null 2>&1
> > > make -j O=/tmp/crypto-build W=1 CC=clang-15 drivers/crypto/virtio/virtio_crypto_akcipher_algs.o
> > > 
> > > I tried clang versions 14, 15, and 16. This is with my home system running
> > > Ubuntu 22.04, no ChromeOS or Google specifics/internals involved. For clang-15,
> > > the version is
> > > 
> > > Ubuntu clang version 15.0.7
> > > Target: x86_64-pc-linux-gnu
> > > Thread model: posix
> > > InstalledDir: /usr/bin
> > 
> > Okay interesting, this warning is hidden behind W=1, which our CI does
> > not test with. Looks like it has been that way since the introduction of
> > these checks in f68f2ff91512 ("fortify: Detect struct member overflows
> > in memcpy() at compile-time").
> > 
> 
> Interestingly the warning is seen in chromeos-5.10, without this patch,
> and without W=1. I guess that must have to do with the revert which is
> finally biting us.
> 
> > I think this is a legitimate warning though. It is complaining about the
> > second memcpy() in virtio_crypto_alg_akcipher_init_session():
> > 
> >    memcpy(&ctrl->u, para, sizeof(ctrl->u));
> > 
> > where ctrl is:
> > 
> >    struct virtio_crypto_op_ctrl_req {
> >            struct virtio_crypto_ctrl_header header;         /*     0    16 */
> >            union {
> >                    struct virtio_crypto_sym_create_session_req sym_create_session; /*    16    56 */
> >                    struct virtio_crypto_hash_create_session_req hash_create_session; /*    16    56 */
> >                    struct virtio_crypto_mac_create_session_req mac_create_session; /*    16    56 */
> >                    struct virtio_crypto_aead_create_session_req aead_create_session; /*    16    56 */
> >                    struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /*    16    56 */
> >                    struct virtio_crypto_destroy_session_req destroy_session; /*    16    56 */
> >                    __u8               padding[56];          /*    16    56 */
> >            } u;                                             /*    16    56 */
> >            union {
> >                    struct virtio_crypto_sym_create_session_req sym_create_session; /*     0    56 */
> >                    struct virtio_crypto_hash_create_session_req hash_create_session; /*     0    56 */
> >                    struct virtio_crypto_mac_create_session_req mac_create_session; /*     0    56 */
> >                    struct virtio_crypto_aead_create_session_req aead_create_session; /*     0    56 */
> >                    struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /*     0    56 */
> >                    struct virtio_crypto_destroy_session_req destroy_session; /*     0    56 */
> >                    __u8                       padding[56];          /*     0    56 */
> >            };
> > 
> > 
> >            /* size: 72, cachelines: 2, members: 2 */
> >            /* last cacheline: 8 bytes */
> >    };
> > 
> > (so size and p_size_field should be 56) and the type of the para
> > parameter in virtio_crypto_alg_akcipher_init_session() is 'void *' but
> > the para passed by reference to
> > virtio_crypto_alg_akcipher_init_session() in virtio_crypto_rsa_set_key()
> > has a type of 'struct virtio_crypto_akcipher_session_para':
> > 
> >    struct virtio_crypto_akcipher_session_para {
> >            __le32                     algo;                 /*     0     4 */
> >            __le32                     keytype;              /*     4     4 */
> >            __le32                     keylen;               /*     8     4 */
> >            union {
> >                    struct virtio_crypto_rsa_session_para rsa; /*    12     8 */
> >                    struct virtio_crypto_ecdsa_session_para ecdsa; /*    12     8 */
> >            } u;                                             /*    12     8 */
> >            union {
> >                    struct virtio_crypto_rsa_session_para rsa;       /*     0     8 */
> >                    struct virtio_crypto_ecdsa_session_para ecdsa;   /*     0     8 */
> >            };
> > 
> > 
> >            /* size: 20, cachelines: 1, members: 4 */
> >            /* last cacheline: 20 bytes */
> >    };
> > 
> > (so q_size_field would be 20 if clang were able to do inlining to see
> > through the 'void *'...?), which would result in the
> > 
> >    __compiletime_lessthan(q_size_field, size)
> > 
> > check succeeding and triggering the warning because 20 < 56, so it does
> > seem like there is an overread of the source buffer here? Adding the
> 
> Looks like it; I think either the passed 'para' should be of type
> virtio_crypto_akcipher_create_session_req() or it should only copy
> sizeof(struct virtio_crypto_akcipher_session_para) bytes.
> 
> Anyway, how did you find that ? Is there a magic trick to find the
> actual code causing the warning ? I am asking because we had seen
> similar warnings before, and it would help to know how to find the
> problematic code.
> 
> Thanks,
> Guenter



Cc: zhenwei pi <pizhenwei@bytedance.com>

Zhenwei I think you wrote most of the code here.
Can you take a look please?
Stack overflows are plus plus ungood.




> > maintainers of the driver and subsystem in question.
> > 
> > Cheers,
> > Nathan

Re: Re: [PATCH 5.10 000/286] 5.10.209-rc1 review

[zhenwei pi]

On 1/29/24 16:46, Michael S. Tsirkin wrote:
> On Fri, Jan 26, 2024 at 03:55:02PM -0800, Guenter Roeck wrote:
>> On 1/26/24 14:35, Nathan Chancellor wrote:
>>> (slimming up the CC list, I don't think this is too relevant to the
>>> wider stable community)
>>>
>>> On Fri, Jan 26, 2024 at 01:01:15PM -0800, Guenter Roeck wrote:
>>>> On 1/26/24 12:34, Nathan Chancellor wrote:
>>>>> On Fri, Jan 26, 2024 at 10:17:23AM -0800, Guenter Roeck wrote:
>>>>>> On 1/26/24 09:51, Greg Kroah-Hartman wrote:
>>>>>>> On Fri, Jan 26, 2024 at 08:46:42AM -0800, Guenter Roeck wrote:
>>>>>>>> On 1/22/24 15:55, Greg Kroah-Hartman wrote:
>>>>>>>>> This is the start of the stable review cycle for the 5.10.209 release.
>>>>>>>>> There are 286 patches in this series, all will be posted as a response
>>>>>>>>> to this one.  If anyone has any issues with these being applied, please
>>>>>>>>> let me know.
>>>>>>>>>
>>>>>>>>> Responses should be made by Wed, 24 Jan 2024 23:56:49 +0000.
>>>>>>>>> Anything received after that time might be too late.
>>>>>>>>>
>>>>>>>> [ ... ]
>>>>>>>>
>>>>>>>>> zhenwei pi <pizhenwei@bytedance.com>
>>>>>>>>>          virtio-crypto: implement RSA algorithm
>>>>>>>>>
>>>>>>>>
>>>>>>>> Curious: Why was this (and its subsequent fixes) backported to v5.10.y ?
>>>>>>>> It is quite beyond a bug fix. Also, unless I am really missing something,
>>>>>>>> the series (or at least this patch) was not applied to v5.15.y, so we now
>>>>>>>> have functionality in v5.10.y which is not in v5.15.y.
>>>>>>>
>>>>>>> See the commit text, it was a dependency of a later fix and documented
>>>>>>> as such.
>>>>>>>
>>>>>>> Having it in 5.10 and not 5.15 is a bit odd, I agree, so patches are
>>>>>>> gladly accepted :)
>>>>>>>
>>>>>>
>>>>>> We reverted the entire series from the merge because it results in a build
>>>>>> failure for us.
>>>>>>
>>>>>> In file included from /home/groeck/src/linux-chromeos/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:10:
>>>>>> In file included from /home/groeck/src/linux-chromeos/include/linux/mpi.h:21:
>>>>>> In file included from /home/groeck/src/linux-chromeos/include/linux/scatterlist.h:5:
>>>>>> In file included from /home/groeck/src/linux-chromeos/include/linux/string.h:293:
>>>>>> /home/groeck/src/linux-chromeos/include/linux/fortify-string.h:512:4: error: call to __read_overflow2_field declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>>>>>>                            __read_overflow2_field(q_size_field, size);
>>>>>
>>>>> For what it's worth, this is likely self inflicted for chromeos-5.10,
>>>>> which carries a revert of commit eaafc590053b ("fortify: Explicitly
>>>>> disable Clang support") as commit c19861d34c003 ("CHROMIUM: Revert
>>>>> "fortify: Explicitly disable Clang support""). I don't see the series
>>>>> that added proper support for clang to fortify in 5.18 that ended with
>>>>> commit 281d0c962752 ("fortify: Add Clang support") in that ChromeOS
>>>>> branch, so this seems somewhat expected.
>>>>>
>>>>
>>>> That explains that ;-). I don't mind if the patches stay in v5.10.y,
>>>> we have them reverted anyway.
>>>>
>>>> The revert was a pure process issue, as you may see when looking into
>>>> commit c19861d34c003, so, yes, I agree that it is self-inflicted damage.
>>>> Still, that doesn't explain why the problem exists in 5.18+.
>>>>
>>>>>> I also see that upstream (starting with 6.1) when trying to build it with clang,
>>>>>> so I guess it is one of those bug-for-bug compatibility things. I really have
>>>>>> no idea what causes it, or why we don't see the problem when building
>>>>>> chromeos-6.1 or chromeos-6.6, but (so far) only with chromeos-5.10 after
>>>>>> merging 5.10.209 into it. Making things worse, the problem isn't _always_
>>>>>> seen. Sometimes I can compile the file in 6.1.y without error, sometimes not.
>>>>>> I have no idea what triggers the problem.
>>>>>
>>>>> Have a .config that reproduces it on upstream? I have not personally
>>>>> seen this warning in my build matrix nor has our continuous-integration
>>>>> matrix (I don't see it in the warning output at the bottom but that
>>>>> could have missed something for some reason) in 6.1:
>>>>>
>>>>
>>>> The following command sequence reproduces the problem for me with all stable
>>>> branches starting with 5.18.y (plus mainline).
>>>>
>>>> rm -rf /tmp/crypto-build
>>>> mkdir /tmp/crypto-build
>>>> make -j CC=clang-15 mrproper >/dev/null 2>&1
>>>> make -j O=/tmp/crypto-build CC=clang-15 allmodconfig >/dev/null 2>&1
>>>> make -j O=/tmp/crypto-build W=1 CC=clang-15 drivers/crypto/virtio/virtio_crypto_akcipher_algs.o
>>>>
>>>> I tried clang versions 14, 15, and 16. This is with my home system running
>>>> Ubuntu 22.04, no ChromeOS or Google specifics/internals involved. For clang-15,
>>>> the version is
>>>>
>>>> Ubuntu clang version 15.0.7
>>>> Target: x86_64-pc-linux-gnu
>>>> Thread model: posix
>>>> InstalledDir: /usr/bin
>>>
>>> Okay interesting, this warning is hidden behind W=1, which our CI does
>>> not test with. Looks like it has been that way since the introduction of
>>> these checks in f68f2ff91512 ("fortify: Detect struct member overflows
>>> in memcpy() at compile-time").
>>>
>>
>> Interestingly the warning is seen in chromeos-5.10, without this patch,
>> and without W=1. I guess that must have to do with the revert which is
>> finally biting us.
>>
>>> I think this is a legitimate warning though. It is complaining about the
>>> second memcpy() in virtio_crypto_alg_akcipher_init_session():
>>>
>>>     memcpy(&ctrl->u, para, sizeof(ctrl->u));
>>>
>>> where ctrl is:
>>>
>>>     struct virtio_crypto_op_ctrl_req {
>>>             struct virtio_crypto_ctrl_header header;         /*     0    16 */
>>>             union {
>>>                     struct virtio_crypto_sym_create_session_req sym_create_session; /*    16    56 */
>>>                     struct virtio_crypto_hash_create_session_req hash_create_session; /*    16    56 */
>>>                     struct virtio_crypto_mac_create_session_req mac_create_session; /*    16    56 */
>>>                     struct virtio_crypto_aead_create_session_req aead_create_session; /*    16    56 */
>>>                     struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /*    16    56 */
>>>                     struct virtio_crypto_destroy_session_req destroy_session; /*    16    56 */
>>>                     __u8               padding[56];          /*    16    56 */
>>>             } u;                                             /*    16    56 */
>>>             union {
>>>                     struct virtio_crypto_sym_create_session_req sym_create_session; /*     0    56 */
>>>                     struct virtio_crypto_hash_create_session_req hash_create_session; /*     0    56 */
>>>                     struct virtio_crypto_mac_create_session_req mac_create_session; /*     0    56 */
>>>                     struct virtio_crypto_aead_create_session_req aead_create_session; /*     0    56 */
>>>                     struct virtio_crypto_akcipher_create_session_req akcipher_create_session; /*     0    56 */
>>>                     struct virtio_crypto_destroy_session_req destroy_session; /*     0    56 */
>>>                     __u8                       padding[56];          /*     0    56 */
>>>             };
>>>
>>>
>>>             /* size: 72, cachelines: 2, members: 2 */
>>>             /* last cacheline: 8 bytes */
>>>     };
>>>
>>> (so size and p_size_field should be 56) and the type of the para
>>> parameter in virtio_crypto_alg_akcipher_init_session() is 'void *' but
>>> the para passed by reference to
>>> virtio_crypto_alg_akcipher_init_session() in virtio_crypto_rsa_set_key()
>>> has a type of 'struct virtio_crypto_akcipher_session_para':
>>>
>>>     struct virtio_crypto_akcipher_session_para {
>>>             __le32                     algo;                 /*     0     4 */
>>>             __le32                     keytype;              /*     4     4 */
>>>             __le32                     keylen;               /*     8     4 */
>>>             union {
>>>                     struct virtio_crypto_rsa_session_para rsa; /*    12     8 */
>>>                     struct virtio_crypto_ecdsa_session_para ecdsa; /*    12     8 */
>>>             } u;                                             /*    12     8 */
>>>             union {
>>>                     struct virtio_crypto_rsa_session_para rsa;       /*     0     8 */
>>>                     struct virtio_crypto_ecdsa_session_para ecdsa;   /*     0     8 */
>>>             };
>>>
>>>
>>>             /* size: 20, cachelines: 1, members: 4 */
>>>             /* last cacheline: 20 bytes */
>>>     };
>>>
>>> (so q_size_field would be 20 if clang were able to do inlining to see
>>> through the 'void *'...?), which would result in the
>>>
>>>     __compiletime_lessthan(q_size_field, size)
>>>
>>> check succeeding and triggering the warning because 20 < 56, so it does
>>> seem like there is an overread of the source buffer here? Adding the
>>
>> Looks like it; I think either the passed 'para' should be of type
>> virtio_crypto_akcipher_create_session_req() or it should only copy
>> sizeof(struct virtio_crypto_akcipher_session_para) bytes.
>>
>> Anyway, how did you find that ? Is there a magic trick to find the
>> actual code causing the warning ? I am asking because we had seen
>> similar warnings before, and it would help to know how to find the
>> problematic code.
>>
>> Thanks,
>> Guenter
> 
> 
> 
> Cc: zhenwei pi <pizhenwei@bytedance.com>
> 
> Zhenwei I think you wrote most of the code here.
> Can you take a look please?
> Stack overflows are plus plus ungood.
> 
> 
> 
> 
>>> maintainers of the driver and subsystem in question.
>>>
>>> Cheers,
>>> Nathan
> 

I can also reproduce this warning by commands on ubuntu-2204:
make -j CC=clang-14 mrproper >/dev/null 2>&1
make -j O=/tmp/crypto-build CC=clang-14 allmodconfig >/dev/null 2>&1
make -j O=/tmp/crypto-build W=1 CC=clang-14 
drivers/crypto/virtio/virtio_crypto_akcipher_algs.o

so sorry on this issue, I think Guenter's suggestion is right(also test 
by these commands), I'll send a fix later.

-- 
zhenwei pi