Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix gcc uninitialized warning in FreeBSD zio_crypt.c #16688

Merged

Conversation

DimitryAndric
Copy link
Contributor

Fix gcc uninitialized warning in FreeBSD zio_crypt.c

With gcc we are seeing the following -Werror warning:

In file included from /workspace/src/sys/contrib/openzfs/include/os/freebsd/spl/sys/sunddi.h:28,
                 from /workspace/src/sys/contrib/openzfs/include/sys/zfs_context.h:69:
In function 'zfs_uio_init',
    inlined from 'zio_do_crypt_data' at /workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c:1690:2:
/workspace/src/sys/contrib/openzfs/include/os/freebsd/spl/sys/uio.h:102:45: error: 'puio_s.uio_offset' is used uninitialized [-Werror=uninitialized]
  102 |                 zfs_uio_soffset(uio) = uio_s->uio_offset;
      |                                        ~~~~~^~~~~~~~~~~~
/workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c: In function 'zio_do_crypt_data':
/workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c:1683:20: note: 'puio_s' declared here
 1683 |         struct uio puio_s, cuio_s;
      |                    ^~~~~~
In function 'zfs_uio_init',
    inlined from 'zio_do_crypt_data' at /workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c:1691:2:
/workspace/src/sys/contrib/openzfs/include/os/freebsd/spl/sys/uio.h:102:45: error: 'cuio_s.uio_offset' is used uninitialized [-Werror=uninitialized]
  102 |                 zfs_uio_soffset(uio) = uio_s->uio_offset;
      |                                        ~~~~~^~~~~~~~~~~~
/workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c: In function 'zio_do_crypt_data':
/workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c:1683:28: note: 'cuio_s' declared here
 1683 |         struct uio puio_s, cuio_s;
      |                            ^~~~~~

Indeed, zfs_uio_init() does:

static __inline void
zfs_uio_init(zfs_uio_t *uio, struct uio *uio_s)
{
        memset(uio, 0, sizeof (zfs_uio_t));
        if (uio_s != NULL) {
                GET_UIO_STRUCT(uio) = uio_s;
                zfs_uio_soffset(uio) = uio_s->uio_offset;
        }
}

while the code in zio_crypt.c has:

/*
 * Primary encryption / decryption entrypoint for zio data.
 */
int
zio_do_crypt_data(boolean_t encrypt, zio_crypt_key_t *key,
    dmu_object_type_t ot, boolean_t byteswap, uint8_t *salt, uint8_t *iv,
    uint8_t *mac, uint_t datalen, uint8_t *plainbuf, uint8_t *cipherbuf,
    boolean_t *no_crypt)
{
        int ret;
        boolean_t locked = B_FALSE;
        uint64_t crypt = key->zk_crypt;
        uint_t keydata_len = zio_crypt_table[crypt].ci_keylen;
        uint_t enc_len, auth_len;
        zfs_uio_t puio, cuio;
        struct uio puio_s, cuio_s;
        uint8_t enc_keydata[MASTER_KEY_MAX_LEN];
        crypto_key_t tmp_ckey, *ckey = NULL;
        freebsd_crypt_session_t *tmpl = NULL;
        uint8_t *authbuf = NULL;

        zfs_uio_init(&puio, &puio_s);
        zfs_uio_init(&cuio, &cuio_s);
        memset(GET_UIO_STRUCT(&puio), 0, sizeof (struct uio));
        memset(GET_UIO_STRUCT(&cuio), 0, sizeof (struct uio));

So between the declaration of puio_s and cuio_s, there is no initialization of these variables before zfs_uio_init() gets called.

Similar to the Linux variant of zio_crypt.c, I think it would be better to memset the structs puio_s and cuis_s before calling zfs_uio_init().

@DimitryAndric
Copy link
Contributor Author

@tsoome @amotin

@DimitryAndric DimitryAndric force-pushed the fix-gcc-uninitialized-warning-1 branch 3 times, most recently from 083583f to e906aec Compare October 25, 2024 16:31
In FreeBSD's `zio_do_crypt_data()`, ensure that two `struct uio`
variables are cleared before copying data out of them. This avoids
accessing garbage data, and fixes gcc `-Wuninitialized` warnings.

Signed-off-by: Dimitry Andric <[email protected]>
@DimitryAndric DimitryAndric force-pushed the fix-gcc-uninitialized-warning-1 branch from e906aec to 853206f Compare October 25, 2024 16:32
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems right. Thanks.

Copy link
Contributor

@tsoome tsoome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Oct 29, 2024
@amotin amotin merged commit 2bf1520 into openzfs:master Oct 29, 2024
20 checks passed
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Nov 1, 2024
In FreeBSD's `zio_do_crypt_data()`, ensure that two `struct uio`
variables are cleared before copying data out of them. This avoids
accessing garbage data, and fixes gcc `-Wuninitialized` warnings.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Toomas Soome <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dimitry Andric <[email protected]>
Closes openzfs#16688
ptr1337 pushed a commit to CachyOS/zfs that referenced this pull request Nov 14, 2024
In FreeBSD's `zio_do_crypt_data()`, ensure that two `struct uio`
variables are cleared before copying data out of them. This avoids
accessing garbage data, and fixes gcc `-Wuninitialized` warnings.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Toomas Soome <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Dimitry Andric <[email protected]>
Closes openzfs#16688
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants