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.

Sign up for GitHub

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

Rectify accessing virtq struct members #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rectify accessing virtq struct members #73

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 18 additions & 16 deletions virtio-snd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,10 +696,10 @@ static void virtio_snd_cb(struct CNFADriver *dev,
}

#define VSND_DESC_CNT 3
static int virtio_snd_desc_handler(virtio_snd_state_t *vsnd,
const virtio_snd_queue_t *queue,
uint32_t desc_idx,
uint32_t *plen)
static int virtio_snd_ctrl_desc_handler(virtio_snd_state_t *vsnd,
const virtio_snd_queue_t *queue,
uint32_t desc_idx,
uint32_t *plen)
{
/* A control message uses at most 3 virtqueue descriptors, where
* the first descriptor contains:
Expand All @@ -713,13 +713,14 @@ static int virtio_snd_desc_handler(virtio_snd_state_t *vsnd,
/* Collect the descriptors */
for (int i = 0; i < VSND_DESC_CNT; i++) {
/* The size of the `struct virtq_desc` is 4 words */
const uint32_t *desc = &vsnd->ram[queue->QueueDesc + desc_idx * 4];
const struct virtq_desc *desc =
(struct virtq_desc *) &vsnd->ram[queue->QueueDesc + desc_idx * 4];

/* Retrieve the fields of current descriptor */
vq_desc[i].addr = desc[0];
vq_desc[i].len = desc[2];
vq_desc[i].flags = desc[3];
desc_idx = desc[3] >> 16; /* vq_desc[desc_cnt].next */
vq_desc[i].addr = desc->addr;
vq_desc[i].len = desc->len;
vq_desc[i].flags = desc->flags;
desc_idx = desc->next;

/* Leave the loop if next-flag is not set */
if (!(vq_desc[i].flags & VIRTIO_DESC_F_NEXT))
Expand Down Expand Up @@ -836,20 +837,21 @@ static int virtio_snd_tx_desc_handler(virtio_snd_state_t *vsnd,
int cnt = 0;
for (;;) {
/* The size of the `struct virtq_desc` is 4 words */
const uint32_t *desc = &vsnd->ram[queue->QueueDesc + desc_idx * 4];
const struct virtq_desc *desc =
(struct virtq_desc *) &vsnd->ram[queue->QueueDesc + desc_idx * 4];
Comment on lines +840 to +841
Copy link

Choose a reason for hiding this comment

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

Consider proper alignment for struct pointer cast

Consider using proper type casting when converting memory addresses to struct pointers. The current cast may cause alignment issues on some architectures. Consider using aligned_cast or ensuring proper alignment.

Code suggestion
Check the AI-generated fix before applying
Suggested change
const struct virtq_desc *desc =
(struct virtq_desc *) &vsnd->ram[queue->QueueDesc + desc_idx * 4];
const struct virtq_desc *desc;
uintptr_t addr = (uintptr_t)&vsnd->ram[queue->QueueDesc + desc_idx * 4];
desc = aligned_cast(struct virtq_desc *, addr);

Code Review Run #2fcc69

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

Comment on lines +840 to +841
Copy link

Choose a reason for hiding this comment

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

Consider adding alignment checks for struct

Consider adding alignment checks when casting memory address to struct virtq_desc*. The current direct cast could lead to unaligned memory access issues on some architectures.

Code suggestion
Check the AI-generated fix before applying
Suggested change
const struct virtq_desc *desc =
(struct virtq_desc *) &vsnd->ram[queue->QueueDesc + desc_idx * 4];
uintptr_t desc_addr = (uintptr_t)&vsnd->ram[queue->QueueDesc + desc_idx * 4];
if (desc_addr % __alignof__(struct virtq_desc) != 0) {
return -1; // Or handle misalignment
}
const struct virtq_desc *desc = (struct virtq_desc *)desc_addr;

Code Review Run #2fcc69

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged


/* Retrieve the fields of current descriptor */
node = (virtq_desc_queue_node_t *) malloc(sizeof(*node));
node->vq_desc.addr = desc[0];
node->vq_desc.len = desc[2];
node->vq_desc.flags = desc[3];
node->vq_desc.addr = desc->addr;
node->vq_desc.len = desc->len;
node->vq_desc.flags = desc->flags;
list_push(&node->q, &q);
desc_idx = desc[3] >> 16; /* vq_desc[desc_cnt].next */
desc_idx = desc->next;

cnt++;

/* Leave the loop if next-flag is not set */
if (!(desc[3] & VIRTIO_DESC_F_NEXT))
if (!(desc->flags & VIRTIO_DESC_F_NEXT))
break;
}

Expand Down Expand Up @@ -1088,7 +1090,7 @@ static bool virtio_snd_reg_write(virtio_snd_state_t *vsnd,
switch (value) {
case VSND_QUEUE_CTRL:
virtio_queue_notify_handler(vsnd, value,
virtio_snd_desc_handler);
virtio_snd_ctrl_desc_handler);
break;
case VSND_QUEUE_TX:
tx_ev_notify++;
Expand Down
Loading